Re: Review Request 68127: Add `ROOT_INTERNET_CURL_UNPRIVILEGED_USER_CommandTaskRootfsWithVolume`.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [68127, 68126, 68125, 68044, 68043, 68041, 68040, 67997, 
67996, 66814, 66813, 66812, 66811, 66840]

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

Error:
The support scripts will be upgraded to Python 3 by July 1st.
Make sure to install Python 3.6 on your machine before.
2018-08-01 02:47:59 URL:https://reviews.apache.org/r/66811/diff/raw/ 
[2756/2756] -> "66811.patch" [1]
error: patch failed: 3rdparty/stout/CMakeLists.txt:30
error: 3rdparty/stout/CMakeLists.txt: patch does not apply

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

- Mesos Reviewbot


On July 31, 2018, 8:35 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68127/
> ---
> 
> (Updated July 31, 2018, 8:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8812
> https://issues.apache.org/jira/browse/MESOS-8812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `ROOT_INTERNET_CURL_UNPRIVILEGED_USER_CommandTaskRootfsWithVolume`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c15a6fad642474765e4ad1952af6cd9ee937379e 
> 
> 
> Diff: https://reviews.apache.org/r/68127/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68123: Avoided unnecessary `Resources::allocations()` call in the allocator.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68123 was successfully built and tested.

Reviews applied: `['68118', '68119', '67444', '68138', '68122', '68123']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2018/mesos-review-68123

- Mesos Reviewbot Windows


On July 31, 2018, 5:17 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68123/
> ---
> 
> (Updated July 31, 2018, 5:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68123/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68132: Batch '/state' requests on Master.

[Benjamin Mahler]

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



Overall the approach looks good, didn't see any bugs, so just minor comments 
below.

* I'm not sure where to put it but it seems we need a TODO to de-duplicate 
response processing when the principal is identical? E.g. if "ben" asks for 
state three times in one batch, ideally we only compute the response for "ben" 
once since they're all identical within a principal?
* Can you document the consistency model in the description?


src/master/http.cpp
Line 2812 (original), 2815 (patched)


Can we keep the existing name? I believe the idea is to have them match the 
path, so "/state" -> Http::state seems ideal as is.



src/master/http.cpp
Lines 2838-2910 (original), 2841-2853 (patched)


It seems a little brittle to inline the batching related logic and deal 
with promises here, could we use a function?

```
.then(... {
  return _state(request, approvers);
})
```

```
Future _state(request, approvers)
{
  bool scheduleBatch = batchedStateRequests.empty();
  
  ... Add entry and grab `future` ...
  
  if (scheduleBatch) { dispatch ... }
  
  return future;
}
```

We could name it differently, e.g. `s/_state/deferStateRequest/`. This way 
the handler doesn't have to inline batching and promise logic.



src/master/http.cpp
Lines 2887-2889 (original), 2849-2851 (patched)


Can you move in the request and the promise (without Owned) here? (the 
lambda will need to be `mutable` for request to be moved here).



src/master/http.cpp
Lines 5173 (patched)


Per my comment above, I guess we could name these like:

```
state
_state
__state
```

Or:

```
state
deferStateRequest
processStateRequestBatch
```

The former seems a little easier to guess the flow, the latter tries to 
name the functions a bit more meaningfully (which can often make the flow 
harder to see from function names alone).



src/master/http.cpp
Lines 5175-5178 (patched)


No need for the special case and the early return? The code will handle 0 
items correctly.

If this is trying to let us know in the future about a bug where the 
batching is firing incorrectly such that there are 0 items, we could CHECK:

```
CHECK(!batchedStateRequests.empty())
  << "Bug in state batching logic";
```

Seems ok without the CHECK to me as well.



src/master/http.cpp
Lines 5319 (patched)


This makes batchedRequest not so const :), might as well have it come in as 
a `BatchedRequest&&` unless `process::async` doesn't support moving yet?



src/master/http.cpp
Lines 5323-5328 (patched)


It seems a little odd to have the lambda have to know about the batch 
struct and do promise setting, instead of just returning the Response:

```
auto response = [this](Owned approvers) {
  ...
  
  return http::OK(...);
}
```

Then this code here is the one that deals with promise setting, e.g.

```
  // Fire off the workers.
  foreach (const BatchedStateRequest& request, batchedStateRequests) {
request.promise.associate(process::async(response, request.approvers));
  }
  
  // Wait for all responses to transition.
  vector> responses;
  foreach (const BatchedStateRequest& request, batchedStateRequests) {
responses.push_back(request.promise.future());
  }
  process::await(responses).await();
```

This lets us keep the response lambda agnostic of batching and we could 
more cleanly move it up in the future.



src/master/http.cpp
Lines 5327 (patched)


Can we move the request in here? If async doesn't support it, can you add a 
TODO?



src/master/master.hpp
Lines 1842 (patched)


Can we avoid Owned and std::move this struct instead of copying it?


- Benjamin Mahler


On July 31, 2018, 5:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68132/
> ---
> 
> (Updated July 31, 2018, 5:24 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9122

Re: Review Request 68094: Add port mapping and network ports isolators sources to CMake.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67751, 68093, 68094]

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

- Mesos Reviewbot


On July 27, 2018, 12:11 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68094/
> ---
> 
> (Updated July 27, 2018, 12:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, James Peach, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8993 and MESOS-8994
> https://issues.apache.org/jira/browse/MESOS-8993
> https://issues.apache.org/jira/browse/MESOS-8994
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the relevant options and source files, but with a fatal
> message if they're used, as we do not yet check for their
> dependencies. This also adds the `mesos-network-helper` executable as
> used by the port mapping isolator.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake f80c265d117c70acda95da6b943cdd3e4e477daa 
>   src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
>   src/slave/containerizer/mesos/CMakeLists.txt 
> ba1f92fe7dd59c34c6dee0bc7ecf6f1b5160eee8 
>   src/tests/CMakeLists.txt 695b6f543847d4270f7004a1768919904975d7a4 
> 
> 
> Diff: https://reviews.apache.org/r/68094/diff/4/
> 
> 
> Testing
> ---
> 
> Pending... There isn't much to test for the isolators added, as they're added 
> with a fatal bailout. Double-checking that without enabling the new options, 
> everything still works as expected.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68134: Avoided reviving on behalf of scheduler after agent reconfiguration.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68134 was successfully built and tested.

Reviews applied: `['67962', '66820', '67776', '67809', '67812', '67813', 
'67814', '66843', '66855', '66856', '66870', '67878', '68085', '67187', 
'67823', '68048', '68134']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2016/mesos-review-68134

- Mesos Reviewbot Windows


On July 31, 2018, 6:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68134/
> ---
> 
> (Updated July 31, 2018, 6:57 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9124
> https://issues.apache.org/jira/browse/MESOS-9124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When agent reconfiguration was enabled in Mesos, the allocator was
> also updated to remove all offer filters associated with an agent when
> that agent's attributes change. In addition, whenever filters for an
> agent are removed, the framework is revived for all roles that it is
> registered in.
> 
> While this ensures that schedulers will have an opportunity to use
> resources on an agent after reconfiguration, modifying the scheduler's
> suppression may put the scheduler in an inconsistent state, where it
> believes it is suppressed in a particular role when it is not.
> 
> This patch eliminates the suppression modification code, while
> keeping the code which removes a reconfigured agent's offer filters.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68134/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 68132: Batch '/state' requests on Master.

[Benjamin Mahler]

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



A couple of comments on the benchmark information before looking at the code, 
these probably belong on the previous review, but since the numbers are only 
shown in this one I'll leave these here:

* Can we compare percentiles (e.g. min, q1, q3, max) across the approaches 
instead of averages? i.e. how much better does min,q1,q3,max get? Averages are 
generally a poor fit for performance data because it doesn't tell us about the 
distribution (e.g. if we make p90 3x worse for a 10% benefit to average that's 
not ok), we can include p50 if we're interested in the half-way point.
* Can you include the cpu model of the box you ran this on? I'm interested in 
how many physical/virtual cores there are.
* Can you also include the regular state query benchmark measurements to make 
sure we're not regressing too much on the single request case? (no need to get 
the non-optimized build numbers).
* Some of the numbers don't look very good, e.g. Before `[min: 1.578161651secs, 
max: 8.789315237secs]` After: `[4.047655443secs, 6.00752698secs]`. Can we see 
the distribution here? Do you understand exactly why the lowest measurement is 
so much higher? Looking at the non-optimized numbers, the minimum didn't get 
worse? Is the data highly variable between runs?
* Can you also include perf data for the optimized run? 
http://mesos.apache.org/documentation/latest/performance-profiling/

- Benjamin Mahler


On July 31, 2018, 5:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68132/
> ---
> 
> (Updated July 31, 2018, 5:24 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9122
> https://issues.apache.org/jira/browse/MESOS-9122
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this patch handlers for '/state' requests are not scheduled
> directly after authorization, but are accumulated and then scheduled
> for later parallel processing.
> 
> This approach allows, if there are N '/state' requests in the Master's
> mailbox and T is the request response time, to block the Master actor
> only once for time O(T) instead of blocking it for time N*T prior to
> this patch.
> 
> This batching technique reduces both the time Master is spending
> answering '/state' requests and the average request response time
> in presence of multiple requests in the Master's mailbox. However,
> for seldom '/state' requests that don't accumulate in the Master's
> mailbox, the response time might increase due to an added trip
> through the mailbox.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6947031da3ce3523408d69d6dac60551a39a4601 
>   src/master/master.hpp 0353d550308816f219aedb6afe15c643fc8bb340 
>   src/master/master.cpp 2af976f7ea7f81d4b06a45ce13286dbd61b9b144 
> 
> 
> Diff: https://reviews.apache.org/r/68132/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.13.5 and various Linux distros.
> 
> Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark.
> 
> Average improvement without optimization: 62%–70%.
> Average improvement with optimization: 17%–62%.
> 
> **[No batching, no 
> optimization](https://dobianchi.files.wordpress.com/2011/11/no-barrique-no-berlusconi.jpg?w=638)**
> ```
> Test setup: 100 agents with a total of 1 running tasks and 1 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 1.102349605secs, 10 responses are in 
> [2.662342ms, 2.143755433secs]
> '/state' response on average took 1.549122019secs, 10 responses are in 
> [494.278454ms, 2.633971927secs]
> 
> Test setup: 1000 agents with a total of 10 running tasks and 10 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 18.436968137secs, 10 responses are in 
> [2.578238ms, 33.210561732secs]
> '/state' response on average took 23.916379537secs, 10 responses are in 
> [5.170660597secs, 43.008091744secs]
> ```
> 
> **With batching but no optimization**
> ```
> Test setup: 100 agents with a total of 1 running tasks and 1 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 417.211022ms, 10 responses are in 
> [4.066901ms, 728.045442ms]
> '/state' response on average took 830.351291ms, 10 respon

Re: Review Request 68122: Fixed couple of typos in the allocator.

[Meng Zhu]

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

(Updated July 31, 2018, 5:08 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
35992474eacb8b14ae57e1dc23307e1542f63cb5 


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

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


Testing
---

Not needed.


Thanks,

Meng Zhu

Review Request 68138: Added tests to ensure correct quota accounting.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added two allocator tests to ensure reserving and
unreserving allocated resources should not affect
quota accounting.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
27fbd9cf0c4442e7675362a806d35bad141ffb6d 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 67444: Made quota consumption tracking event-driven in the allocator.

[Meng Zhu]

> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > Looks like a great cleanup!
> > 
> > Persist tends to carry the connotation of writing something to durable 
> > storage. How about:
> > 
> > ```
> > Made quota consumption tracking event-driven in the allocator.
> > 
> > The allocator needs to keep track of role consumed quota
> > to maintain quota headroom. Currently this info is
> > constructed on the fly prior to each allocation cycle.
> > 
> > This patch lets the allocator track quota consumption
> > across allocation cycles in an event-driven manner to improve
> > performance and reduce code complexity.
> > ```

Sounds good. Thanks!


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1404 (patched)
> > 
> >
> > It wasn't clear to me when reading this why it needs to be done on each 
> > agent, so we should probably clarify that?
> > 
> > ```
> > // Lastly, subtract allocated reservations. This needs to be done on a 
> > per-agent basis because ...
> > ```

I do not think we have anything interesting to say here. We subtract on a 
per-agent basis because currently there is no aggregated bookkeeping info. In 
the future, it is possible for us to track aggregated allocated reservations 
and subtract all at once. Dropping.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1405-1406 (patched)
> > 
> >
> > We probably want a TODO to optimize this by avoiding the copy by 
> > returning a const reference to the map?

Looks like the `quotaRoleSorter->allocation(role)` is already returning a const 
ref, must be a slip earlier. Will update these in an immediate followup patch.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1385-1390 (original), 1413-1418 (patched)
> > 
> >
> > Just an unrelated observation, this note looks misleading, since the 
> > master does try to rescind offers to re-balance.

"Re-balancing" means a change of allocation? Anyway, added a todo here:

  // TODO(mzhu): Re-evaluate the above assumption and consider triggering an
  // allocation here.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1439-1440 (patched)
> > 
> >
> > It looks like it follows the same order as operations in setQuota, so I 
> > would expect this to be done after metrics.removeQuota. Any reason not to?
> > 
> > Actually, it seems like the metric should get added after quota 
> > tracking and removed before quota tracking is removed, since I would 
> > imagine the metrics look at the tracking information. It looks like we 
> > currently only expose 'allocated' instead of 'consumed' for now, but we 
> > probably want to expose 'consumed' soon, is there a ticket?

Filed MESOS-9123 for exposing role consumed quota.

Updated `metrics.setQuota` to be done after the tracking info update. But for 
`metrics.removeQuota`, it does not depend on any of the tracking info since it 
is all about removing the metric entries. I think it can be done either before 
or after the tracking info update. For consistency, let's update the metrics 
after the tracking info update in both add and remove.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2586-2588 (patched)
> > 
> >
> > It's pretty hard to reason from here about whether this is correct. For 
> > example, how do we know that these quantities were not already tracked 
> > because they were allocated prior to becoming reserved? If that invariant 
> > doesn't hold we'll double count?
> 
> Meng Zhu wrote:
> Thanks for catching this!
> 
> The invariant should be:
> (1) when tracking reservation, only track unallocated reservations
> (2) when track allocation, only track unreserved allocations
> 
> (2) is already being done. (1) is hard to do given the current 
> abstraction, we will need more than raw "reservations". Will need more 
> refactoring on the Slave struct in the allocator to make this work and 
> readable. Will come back to this later.
> 
> In addition, this reminds me that we do not have tests for the above 
> scenario yet. Will add some.

Now fixed.


- Meng


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

Re: Review Request 67444: Made quota consumption tracking event-driven in the allocator.

[Meng Zhu]

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

(Updated July 31, 2018, 4:56 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Made quota consumption tracking event-driven in the allocator.


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


Repository: mesos


Description (updated)
---

The allocator needs to keep track of role consumed quota
to maintain quota headroom. Currently this info is
constructed on the fly prior to each allocation cycle.

This patch lets the allocator track quota consumption
across allocation cycles in an event-driven manner to improve
performance and reduce code complexity.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
0cd2fac17f09110c46b8540523a9c935f156f858 
  src/master/allocator/mesos/hierarchical.cpp 
35992474eacb8b14ae57e1dc23307e1542f63cb5 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 67187: Tested per-framework task state metrics.

[Greg Mann]

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

(Updated July 31, 2018, 11:25 p.m.)


Review request for mesos, Gastón Kleiman and Gilbert Song.


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


Repository: mesos


Description
---

This patch adds `MasterTest.TaskStateMetrics`, which verifies that
per-framework task state metrics for both terminal and active task
states report correct values, even after agent reregistration.


Diffs (updated)
-

  src/tests/master_tests.cpp 44b0ac39f87c6415e130c5e7f505428642739311 


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

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


Testing
---

The new test was run ~10,000 times with no failures.


Thanks,

Greg Mann

Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 67931.

Failed command: `python.exe .\support\python3\apply-reviews.py -n -r 67931`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2017/mesos-review-67932

Relevant logs:

- 
[apply-review-67931-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2017/mesos-review-67932/logs/apply-review-67931-stdout.log):

```
error: patch failed: src/uri/fetchers/docker.hpp:58
error: src/uri/fetchers/docker.hpp: patch does not apply
```

- Mesos Reviewbot Windows


On July 31, 2018, 9:41 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67932/
> ---
> 
> (Updated July 31, 2018, 9:41 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `tar` command cannot work successfully on Windows, so use `wclayer`
> instead. Note that the folder generated from extraction also cannot be
> deleted by `rmdir`, so the GC is also changed to use `wclayer remove`.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc 
>   src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 85aad25ac8ecfda125be85fb46d882c3982f3930 
> 
> 
> Diff: https://reviews.apache.org/r/67932/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 68085: Extended a v1 test helper.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On July 27, 2018, 9:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68085/
> ---
> 
> (Updated July 27, 2018, 9:23 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended a v1 test helper.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 8142f3f3ab8e4e9230efdcd2e6846d0e0bd293b2 
> 
> 
> Diff: https://reviews.apache.org/r/68085/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67187: Tested per-framework task state metrics.

[Gastón Kleiman]

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


Fix it, then Ship it!





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


Nit: do we nee the `mesos::internal::` prefix? No other test seems to need 
it.



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


Ditto prefix.


- Gastón Kleiman


On July 30, 2018, 6:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67187/
> ---
> 
> (Updated July 30, 2018, 6:53 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Gilbert Song.
> 
> 
> Bugs: MESOS-8847
> https://issues.apache.org/jira/browse/MESOS-8847
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds `MasterTest.TaskStateMetrics`, which verifies that
> per-framework task state metrics for both terminal and active task
> states report correct values, even after agent reregistration.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 44b0ac39f87c6415e130c5e7f505428642739311 
> 
> 
> Diff: https://reviews.apache.org/r/67187/diff/8/
> 
> 
> Testing
> ---
> 
> The new test was run ~10,000 times with no failures.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.

[Andrew Schwartzmeyer]

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


Ship it!




I'll clean up for Mesos whitespace/formatting/commenting etc., and double-check 
the schema, but otherwise LGTM

- Andrew Schwartzmeyer


On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated July 31, 2018, 2:41 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
>   src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67930: Get tests ready for Windows UCR development.

[Andrew Schwartzmeyer]

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


Ship it!




I'll clean up for Mesos whitespace/formatting/commenting etc., but otherwise 
LGTM.

- Andrew Schwartzmeyer


On July 31, 2018, 2:42 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67930/
> ---
> 
> (Updated July 31, 2018, 2:42 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled Internet test environment on Windows. Disabled Internet
> HealthCheckTests on Windows, since they require complete development.
> Modified DockerFetcherPluginTest to fetch `microsoft/nanoserver` for
> more extensive test for fetcher on Windows.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp 3b84c0a413193badbbdb6d3ee71d74f3ab85c90b 
>   src/tests/health_check_tests.cpp 34102e2e3ff90c194bea83ae8a702181121d6522 
>   src/tests/uri_fetcher_tests.cpp 816d8ec2503ea79d069c062dfa2f01224b263bf6 
> 
> 
> Diff: https://reviews.apache.org/r/67930/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67984: Windows: Added CMake logic to download and "install" `wclayer.exe`.

[Andrew Schwartzmeyer]

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


Ship it!




I'll make sure this gets committed with original authorship (me); I thought RB 
would preserve it, but apparently not, I guess it takes it from the user 
submitting. Oh well.

- Andrew Schwartzmeyer


On July 31, 2018, 2:42 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67984/
> ---
> 
> (Updated July 31, 2018, 2:42 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added CMake logic to download and "install" `wclayer.exe`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   src/slave/CMakeLists.txt e5fe32a265e310f568139127ee6e5f441590985c 
> 
> 
> Diff: https://reviews.apache.org/r/67984/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.

[Andrew Schwartzmeyer]

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


Ship it!




I'll clean up for Mesos whitespace/formatting/commenting etc., but otherwise 
LGTM.

- Andrew Schwartzmeyer


On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67932/
> ---
> 
> (Updated July 31, 2018, 2:41 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `tar` command cannot work successfully on Windows, so use `wclayer`
> instead. Note that the folder generated from extraction also cannot be
> deleted by `rmdir`, so the GC is also changed to use `wclayer remove`.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc 
>   src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 85aad25ac8ecfda125be85fb46d882c3982f3930 
> 
> 
> Diff: https://reviews.apache.org/r/67932/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.

[Joerg Schad]

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


Ship it!




Ship It!

- Joerg Schad


On July 31, 2018, 4:34 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68111/
> ---
> 
> (Updated July 31, 2018, 4:34 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'MesosCon 2018 CFP is now open!' blog post.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68111/diff/4/
> 
> 
> Testing
> ---
> 
> Used `./mesos-website-dev.sh` to verify that the website is properly rendered.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.

[Andrew Schwartzmeyer]

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


Ship it!




Tested sponsor email, it works!

- Andrew Schwartzmeyer


On July 31, 2018, 9:34 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68111/
> ---
> 
> (Updated July 31, 2018, 9:34 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'MesosCon 2018 CFP is now open!' blog post.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68111/diff/4/
> 
> 
> Testing
> ---
> 
> Used `./mesos-website-dev.sh` to verify that the website is properly rendered.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.

[Andrew Schwartzmeyer]

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




src/common/command_utils.cpp
Line 186 (original), 186 (patched)


:)


- Andrew Schwartzmeyer


On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67932/
> ---
> 
> (Updated July 31, 2018, 2:41 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `tar` command cannot work successfully on Windows, so use `wclayer`
> instead. Note that the folder generated from extraction also cannot be
> deleted by `rmdir`, so the GC is also changed to use `wclayer remove`.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc 
>   src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 85aad25ac8ecfda125be85fb46d882c3982f3930 
> 
> 
> Diff: https://reviews.apache.org/r/67932/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67984: Windows: Added CMake logic to download and "install" `wclayer.exe`.

[Liangyu Zhao via Review Board]

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

(Updated July 31, 2018, 9:42 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.


Repository: mesos


Description
---

Windows: Added CMake logic to download and "install" `wclayer.exe`.


Diffs (updated)
-

  3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
  src/slave/CMakeLists.txt e5fe32a265e310f568139127ee6e5f441590985c 


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

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


Testing
---


Thanks,

Liangyu Zhao

Re: Review Request 67930: Get tests ready for Windows UCR development.

[Liangyu Zhao via Review Board]

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

(Updated July 31, 2018, 9:42 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.


Repository: mesos


Description
---

Enabled Internet test environment on Windows. Disabled Internet
HealthCheckTests on Windows, since they require complete development.
Modified DockerFetcherPluginTest to fetch `microsoft/nanoserver` for
more extensive test for fetcher on Windows.


Diffs (updated)
-

  src/tests/environment.cpp 3b84c0a413193badbbdb6d3ee71d74f3ab85c90b 
  src/tests/health_check_tests.cpp 34102e2e3ff90c194bea83ae8a702181121d6522 
  src/tests/uri_fetcher_tests.cpp 816d8ec2503ea79d069c062dfa2f01224b263bf6 


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

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


Testing
---


Thanks,

Liangyu Zhao

Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.

[Liangyu Zhao via Review Board]

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

(Updated July 31, 2018, 9:41 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.


Repository: mesos


Description
---

The `tar` command cannot work successfully on Windows, so use `wclayer`
instead. Note that the folder generated from extraction also cannot be
deleted by `rmdir`, so the GC is also changed to use `wclayer remove`.


Diffs (updated)
-

  src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc 
  src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
85aad25ac8ecfda125be85fb46d882c3982f3930 


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

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


Testing
---


Thanks,

Liangyu Zhao

Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.

[Liangyu Zhao via Review Board]

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

(Updated July 31, 2018, 9:41 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.


Repository: mesos


Description
---

Support parsing schema 2 and fetching blob with multiple URLs as
specified in schema 2. Update `curl` version to 7.61.0 because of bug
encountered in version 7.57.0.


Diffs (updated)
-

  3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
  3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
  include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
  include/mesos/docker/v2_2.hpp PRE-CREATION 
  include/mesos/docker/v2_2.proto PRE-CREATION 
  include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 
  src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
  src/slave/containerizer/mesos/containerizer.cpp 
98129d006cda9b65804b518619b6addc8990410a 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
  src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
  src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
  src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 
  src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 


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

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


Testing
---


Thanks,

Liangyu Zhao

Re: Review Request 68120: UI: Pull up the leader URL generation to a top-level function.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68120 was successfully built and tested.

Reviews applied: `['68120']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2014/mesos-review-68120

- Mesos Reviewbot Windows


On July 31, 2018, 7:26 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68120/
> ---
> 
> (Updated July 31, 2018, 7:26 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This path pulls up the leader URL prefix generation to a top-level
> function in order to have it re-used across controllers.
> 
> This also ensures that everything shown is consistently coming
> from the leading master, specifically, now the log viewer shows
> the leading master's log (and the UI messaging is updated in this
> patch accordingly).
> 
> Having this logic in just one place makes it easier to modify it,
> e.g., to make it possible to use the UI via a reverse proxy.
> 
> 
> Diffs
> -
> 
>   src/webui/app/controllers.js 7c228a1629d7b1dcce56b432f042f02d3ec1583b 
>   src/webui/app/home.html ff965952d7a8d84c9dcfd5d6e7e045cd72bc 
> 
> 
> Diff: https://reviews.apache.org/r/68120/diff/2/
> 
> 
> Testing
> ---
> 
> manual testing
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

[Chun-Hung Hsiao]

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


Ship it!





src/slave/http.cpp
Lines 1831-1833 (patched)


It seems we can hoist this outside the loop. However we might want to 
consider having finer-grained ACLs in the future so I'm fine with keeping it as 
is.


- Chun-Hung Hsiao


On July 31, 2018, 10:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated July 31, 2018, 10:19 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8314
> https://issues.apache.org/jira/browse/MESOS-8314
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
>   src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 
> 
> 
> Diff: https://reviews.apache.org/r/68104/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

[Chun-Hung Hsiao]

> On July 31, 2018, 3:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1821-1822 (patched)
> > 
> >
> > The following can be fit into 80 characters:
> > ```
> > [this, acceptType](const Owned& approvers) -> 
> > Response {
> > ```
> > Or for here it seems harmless to just use `[=]`. I'm fine with either 
> > though.
> 
> Benjamin Bannier wrote:
> That line seems to be exactly 81 characters long, so it looks like we do 
> need to split somewhere. I now manually adjusted this to not split the 
> capture list (the previous version was formated by our `clang-format`).
> 
> I don't think that implicitly capturing pointers to not immediately 
> invoked lambdas is safe style, much less when capturing `this` like here. 
> I'll keep the explicit capture.

You're right about the line length. I must have made a mistake.

It seems that it's more common to do the following indentation in our codebase:
```
[this, acceptType](
const Owned& approvers) -> Response {
```


> On July 31, 2018, 3:38 a.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 7002-7004 (original), 7002-7021 (patched)
> > 
> >
> > How about the following:
> > ```
> > slave::Flags slaveFlags = CreateSlaveFlags();
> > slaveFlags.authenticate_http_readwrite = true;
> > 
> > {
> >   // `DEFAULT_CREDENTIAL_2` is not allowed to view any resource 
> > provider.
> >   mesos::ACL::ViewResourceProvider* acl =
> > slaveFlags.acls->add_view_resource_providers();
> >   
> > acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL_2.principal());
> >   acl->mutable_resource_providers()->set_type(mesos::ACL::Entity::NONE);
> > {
> > 
> > Try> slave = StartSlave(&detector, slaveFlags);
> > ```
> 
> Benjamin Bannier wrote:
> Great suggestion, applied.
> 
> I did not set `authenticate_http_readwrite` though since it is not 
> required.

Are you sure about this? The test won't pass if I don't turn this flag on:
```
[ RUN  ] ContentType/AgentAPITest.GetResourceProviders/0
../src/tests/api_tests.cpp:7091: Failure
  Expected: 0
To be equal to: v1Response->get_resource_providers().resource_providers_size()
  Which is: 1
[  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/0, where GetParam() 
= application/x-protobuf (192 ms)
[ RUN  ] ContentType/AgentAPITest.GetResourceProviders/1
../src/tests/api_tests.cpp:7091: Failure
  Expected: 0
To be equal to: v1Response->get_resource_providers().resource_providers_size()
  Which is: 1
[  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/1, where GetParam() 
= application/json (164 ms)
```

Also, we don't need to set up the ACL for `DEFAULT_CREDENTIAL` since the 
permissive mode is turned on by default.


- Chun-Hung


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


On July 31, 2018, 10:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated July 31, 2018, 10:19 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8314
> https://issues.apache.org/jira/browse/MESOS-8314
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
>   src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 
> 
> 
> Diff: https://reviews.apache.org/r/68104/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68114: Fixed a gRPC compilation issue for Clang.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [68091, 68074, 68092, 68114]

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

- Mesos Reviewbot


On July 31, 2018, 12:05 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68114/
> ---
> 
> (Updated July 31, 2018, 12:05 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8395
> https://issues.apache.org/jira/browse/MESOS-8395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When compiling gRPC with Clang, there are some array-out-of-bound
> warnings due to the use of GLIBC's `__strcmp_cg` macro in the c-ares
> library. With `-Werror` on, these warnings would stop gRPC from
> compiling. This patch ignores such errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 26e5d55561731ff03639df31562bb835d9687339 
> 
> 
> Diff: https://reviews.apache.org/r/68114/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check` with clang on ubuntu 16.04, which triggers the warnings.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 68103: Added an authorizer action for viewing of resource provider information.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On July 31, 2018, 10:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68103/
> ---
> 
> (Updated July 31, 2018, 10:19 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8314
> https://issues.apache.org/jira/browse/MESOS-8314
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an authorizer action for viewing of resource provider information.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 1777c04316bfd7493894f0e782f170fe8437aafe 
>   include/mesos/authorizer/authorizer.proto 
> 8b5fa09f389ca4c26f8390d35af32c4cdd561418 
>   src/authorizer/local/authorizer.cpp 
> abf5b4663cd517fb6d69b5373dd0e6520e87cf8e 
>   src/tests/authorization_tests.cpp 41ecac29a53f6ad9553cbf0a1121e1f3cff50df8 
> 
> 
> Diff: https://reviews.apache.org/r/68103/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67916 was successfully built and tested.

Reviews applied: `['67916']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2013/mesos-review-67916

- Mesos Reviewbot Windows


On July 31, 2018, 6:02 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> ---
> 
> (Updated July 31, 2018, 6:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
> 
> This patch file was generated with `git format-patch` for the commit
> `f66ab00704cd47e4e63ef6d425ca14b9192aaebb`.
> 
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 
>   3rdparty/googletest-release-1.8.0.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67916/diff/3/
> 
> 
> Testing
> ---
> 
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following 
> (temporary) CMake changes:
> 
> ```
>  # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
>  # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
> 
> After adding the necessary compile interface definition, `ninja tests` 
> successfully built on Windows with this patch applied, after not building 
> before the patch.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68120: UI: Pull up the leader URL generation to a top-level function.

[Benjamin Mahler]

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

(Updated July 31, 2018, 7:26 p.m.)


Review request for mesos, Armand Grillet and Gastón Kleiman.


Changes
---

Updated the html to indicate that the log viewer is pointing to the leading 
master's log.


Repository: mesos


Description (updated)
---

This path pulls up the leader URL prefix generation to a top-level
function in order to have it re-used across controllers.

This also ensures that everything shown is consistently coming
from the leading master, specifically, now the log viewer shows
the leading master's log (and the UI messaging is updated in this
patch accordingly).

Having this logic in just one place makes it easier to modify it,
e.g., to make it possible to use the UI via a reverse proxy.


Diffs (updated)
-

  src/webui/app/controllers.js 7c228a1629d7b1dcce56b432f042f02d3ec1583b 
  src/webui/app/home.html ff965952d7a8d84c9dcfd5d6e7e045cd72bc 


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

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


Testing
---

manual testing


Thanks,

Benjamin Mahler

Re: Review Request 67813: Added per-framework metrics for task states.

[Greg Mann]

> On July 11, 2018, 11:48 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Line 2415 (original), 2415 (patched)
> > 
> >
> > NOTE: should review this patch to ensure that we don't have a 
> > double-counting bug after master failover, based on recent testing. 
> > Observed with TASK_RUNNING in particular.

Looks like there is no double-counting issue. Dropping this one.


- Greg


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


On July 18, 2018, 1:35 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67813/
> ---
> 
> (Updated July 18, 2018, 1:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Gastón Kleiman, 
> Gilbert Song, and Vinod Kone.
> 
> 
> Bugs: MESOS-8847
> https://issues.apache.org/jira/browse/MESOS-8847
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per-framework metrics for task states.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2ce71dca52245b41533728a7564c65daa135b224 
>   src/master/master.cpp 487ee349ef0cd78903ff6bcdea3f24688810cdc5 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
> 
> 
> Diff: https://reviews.apache.org/r/67813/diff/5/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 66870: Added per-framework metrics for suppressed roles.

[Greg Mann]

> On July 25, 2018, 11:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 734-758 (original), 747-772 (patched)
> > 
> >
> > I'm not familiar with this, but it reads as wrong. Why does clearing 
> > the agent filters modify the framework's suppressed roles? That breaks the 
> > API contract in which frameworks are in control of their suppression state.
> > 
> > I don't think we should tweak their suppression state based on events, 
> > we should instead expose those events to schedulers and let them decide.
> > 
> > Can you follow up with a ticket / fix for this or ask (benno i think?) 
> > to follow up?

I've posted a patch at the end of this chain to address this issue: 
https://reviews.apache.org/r/68134/

Thanks for bringing this up, bmahler!!


- Greg


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


On July 26, 2018, 6:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66870/
> ---
> 
> (Updated July 26, 2018, 6:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per-framework metrics for suppressed roles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 7b4e9db3435b51c9ce025b7b522e10db4b907ebb 
>   src/master/allocator/mesos/metrics.hpp 
> 6d386225c301d5ab44f3cc0ecdd1478fb5162e5b 
>   src/master/allocator/mesos/metrics.cpp 
> 82990b2dc0b827a43a392d898667eaf58c77ea36 
> 
> 
> Diff: https://reviews.apache.org/r/66870/diff/7/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 68134: Avoided reviving on behalf of scheduler after agent reconfiguration.

[Greg Mann]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

When agent reconfiguration was enabled in Mesos, the allocator was
also updated to remove all offer filters associated with an agent when
that agent's attributes change. In addition, whenever filters for an
agent are removed, the framework is revived for all roles that it is
registered in.

While this ensures that schedulers will have an opportunity to use
resources on an agent after reconfiguration, modifying the scheduler's
suppression may put the scheduler in an inconsistent state, where it
believes it is suppressed in a particular role when it is not.

This patch eliminates the suppression modification code, while
keeping the code which removes a reconfigured agent's offer filters.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
35992474eacb8b14ae57e1dc23307e1542f63cb5 


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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.

[Andrew Schwartzmeyer]

> On July 30, 2018, 4:39 p.m., Andrew Schwartzmeyer wrote:
> > include/mesos/uri/fetcher.hpp
> > Lines 101 (patched)
> > 
> >
> > I am not convinced we need to be passing this as a shared_ptr, wouldn't 
> > const-ref be just fine?

Spoke offline; the reason this is the right choice is that this vector of 
strings is later used in several async lambdas. So by sharing a pointer to the 
vector instead, we utilize the ref counting of `shared_ptr` to avoid having to 
copy the vector for each lambda, and the size of the vector is ~10KB, so it's 
reasonable to want to avoid this.


> On July 30, 2018, 4:39 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 1016-1017 (patched)
> > 
> >
> > Are we just skipping a failed blob here and trying to process the rest? 
> > Does that make sense to do?
> 
> Liangyu Zhao wrote:
> Yes, the code will try each url until one succeeds. If all fail, the 
> fetch will be considered failed.

I'm not sure I understand why that works. Are these multiple URLs for the same 
blob? I would have that any single failed fetch would result in a total failure.


- Andrew


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


On July 30, 2018, 3:40 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated July 30, 2018, 3:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
>   src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.

[Andrew Schwartzmeyer]

> On July 30, 2018, 5:30 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
> > Line 381 (original), 388 (patched)
> > 
> >
> > Did we change behavior here? This is in a loop, so the original code 
> > was constantly adding to the front, whereas now we're adding the back (IMHO 
> > original code smells a bit funny).
> 
> Liangyu Zhao wrote:
> Nope, in line 469, I reverse the vector. It's just not cool to 
> continuously add stuff at front in vector.

Ah, I see. Can you update the location of the comment above now? I agree, it's 
not cool to always prepend to a vector.


> On July 30, 2018, 5:30 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp
> > Lines 379-380 (patched)
> > 
> >
> > I take it that the `wclayer` command in `wclayer_remove` is supposed to 
> > remove the directory?
> 
> Liangyu Zhao wrote:
> Yeah, there is some privilege issues that `rmdir` does not work here. To 
> remove the directory, we have to use `wclayer`.

That's fine; please add a comment though to explain the discrepancy with the 
Linux code.


> On July 30, 2018, 5:30 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp
> > Lines 490 (patched)
> > 
> >
> > Last I remember, `flags.docker_store_dir` was some Linux path; did we 
> > get that fixed (or am I misremembering)?
> 
> Liangyu Zhao wrote:
> I believe it has already been fixed. I have run the agent and log this. 
> It turns out to be a Windows path.

Yup, looks like it: `path::join(os::temp(), "mesos", "store", "docker")`.


- Andrew


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


On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67932/
> ---
> 
> (Updated July 18, 2018, 2:27 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `tar` command cannot work successfully on Windows, so use `wclayer`
> instead. Note that the folder generated from extraction also cannot be
> deleted by `rmdir`, so the GC is also changed to use `wclayer remove`.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc 
>   src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 85aad25ac8ecfda125be85fb46d882c3982f3930 
> 
> 
> Diff: https://reviews.apache.org/r/67932/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68111 was successfully built and tested.

Reviews applied: `['68111']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2012/mesos-review-68111

- Mesos Reviewbot Windows


On July 31, 2018, 4:34 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68111/
> ---
> 
> (Updated July 31, 2018, 4:34 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'MesosCon 2018 CFP is now open!' blog post.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68111/diff/4/
> 
> 
> Testing
> ---
> 
> Used `./mesos-website-dev.sh` to verify that the website is properly rendered.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.

[Liangyu Zhao via Review Board]

> On July 31, 2018, 12:30 a.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
> > Line 381 (original), 388 (patched)
> > 
> >
> > Did we change behavior here? This is in a loop, so the original code 
> > was constantly adding to the front, whereas now we're adding the back (IMHO 
> > original code smells a bit funny).

Nope, in line 469, I reverse the vector. It's just not cool to continuously add 
stuff at front in vector.


> On July 31, 2018, 12:30 a.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
> > Line 430 (original), 469 (patched)
> > 
> >
> > Why do we have to do this now?

Explained above.


> On July 31, 2018, 12:30 a.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp
> > Lines 379-380 (patched)
> > 
> >
> > I take it that the `wclayer` command in `wclayer_remove` is supposed to 
> > remove the directory?

Yeah, there is some privilege issues that `rmdir` does not work here. To remove 
the directory, we have to use `wclayer`.


> On July 31, 2018, 12:30 a.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp
> > Lines 490 (patched)
> > 
> >
> > Last I remember, `flags.docker_store_dir` was some Linux path; did we 
> > get that fixed (or am I misremembering)?

I believe it has already been fixed. I have run the agent and log this. It 
turns out to be a Windows path.


- Liangyu


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


On July 18, 2018, 9:27 a.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67932/
> ---
> 
> (Updated July 18, 2018, 9:27 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `tar` command cannot work successfully on Windows, so use `wclayer`
> instead. Note that the folder generated from extraction also cannot be
> deleted by `rmdir`, so the GC is also changed to use `wclayer remove`.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc 
>   src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 85aad25ac8ecfda125be85fb46d882c3982f3930 
> 
> 
> Diff: https://reviews.apache.org/r/67932/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Andrew Schwartzmeyer]

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

(Updated July 31, 2018, 11:02 a.m.)


Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.


Changes
---

Used original upstream patch as GNU Patch is capable of applying it, though 
`git cherry-pick` (my original method) could not.


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


Repository: mesos


Description (updated)
---

Per MESOS-8990, our Google Test dependency needs a patch from
upstream, https://github.com/google/googletest/pull/1620, in order to
continue building with the next version of MSVC (and potentially other
compilers).

This patch file was generated with `git format-patch` for the commit
`f66ab00704cd47e4e63ef6d425ca14b9192aaebb`.

Additionally, we need to define `GTEST_LANG_CXX11=1` when including
the GoogleTest headers such that the patch is used.


Diffs (updated)
-

  3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 
  3rdparty/googletest-release-1.8.0.patch PRE-CREATION 


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

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


Testing
---

Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following 
(temporary) CMake changes:

```
 # Set the default standard to C++11 for all targets.
-set(CMAKE_CXX_STANDARD 11)
-set(CMAKE_CXX_STANDARD_REQUIRED ON)
+# set(CMAKE_CXX_STANDARD 11)
+# set(CMAKE_CXX_STANDARD_REQUIRED ON)
 # Do not use, for example, `-std=gnu++11`.
-set(CMAKE_CXX_EXTENSIONS OFF)
+# set(CMAKE_CXX_EXTENSIONS OFF)
+add_compile_options(/std:c++latest)
+add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
+add_definitions(-D_HAS_AUTO_PTR_ETC=1)
+add_definitions(-D_HAS_TR1_NAMESPACE=1)
+add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
+add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
```

After adding the necessary compile interface definition, `ninja tests` 
successfully built on Windows with this patch applied, after not building 
before the patch.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 68053: Call any function in a specified namespace.

[Sergey Urbanovich]

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

(Updated July 31, 2018, 5:30 p.m.)


Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Jie Yu, and Qian 
Zhang.


Changes
---

Minor changes.


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


Repository: mesos


Description
---

The NamespaceRunner runs any function in a specified namespace. To do
that it manages a separate thread which would be re-associated with
that namespace.


Diffs (updated)
-

  src/linux/ns.hpp 0b4136bd3cc2d3e0cfee163d89469558e699b5f2 
  src/tests/containerizer/ns_tests.cpp fa4349e29b975550e05b00a1f848a24cd8e4f0de 


Diff: https://reviews.apache.org/r/68053/diff/6/

Changes: https://reviews.apache.org/r/68053/diff/5-6/


Testing
---

bin/mesos-tests.sh --verbose --gtest_filter="NsTest*" --gtest_break_on_failure 
--gtest_repeat=100


Thanks,

Sergey Urbanovich

Re: Review Request 68132: Batch '/state' requests on Master.

[Alexander Rukletsov]

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

(Updated July 31, 2018, 5:24 p.m.)


Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

With this patch handlers for '/state' requests are not scheduled
directly after authorization, but are accumulated and then scheduled
for later parallel processing.

This approach allows, if there are N '/state' requests in the Master's
mailbox and T is the request response time, to block the Master actor
only once for time O(T) instead of blocking it for time N*T prior to
this patch.

This batching technique reduces both the time Master is spending
answering '/state' requests and the average request response time
in presence of multiple requests in the Master's mailbox. However,
for seldom '/state' requests that don't accumulate in the Master's
mailbox, the response time might increase due to an added trip
through the mailbox.


Diffs
-

  src/master/http.cpp 6947031da3ce3523408d69d6dac60551a39a4601 
  src/master/master.hpp 0353d550308816f219aedb6afe15c643fc8bb340 
  src/master/master.cpp 2af976f7ea7f81d4b06a45ce13286dbd61b9b144 


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


Testing (updated)
---

`make check` on Mac OS 10.13.5 and various Linux distros.

Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark.

Average improvement without optimization: 62%–70%.
Average improvement with optimization: 17%–62%.

**[No batching, no 
optimization](https://dobianchi.files.wordpress.com/2011/11/no-barrique-no-berlusconi.jpg?w=638)**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.102349605secs, 10 responses are in 
[2.662342ms, 2.143755433secs]
'/state' response on average took 1.549122019secs, 10 responses are in 
[494.278454ms, 2.633971927secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 18.436968137secs, 10 responses are in 
[2.578238ms, 33.210561732secs]
'/state' response on average took 23.916379537secs, 10 responses are in 
[5.170660597secs, 43.008091744secs]
```

**With batching but no optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 417.211022ms, 10 responses are in 
[4.066901ms, 728.045442ms]
'/state' response on average took 830.351291ms, 10 responses are in 
[459.033455ms, 1.208880892secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 5.439950928secs, 10 responses are in 
[3.246906ms, 9.343994388secs]
'/state' response on average took 16.764607823secs, 10 responses are in 
[4.980333091secs, 18.461983916secs]
```

**No batching but `-O3` optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 2.396221ms, 10 responses are in [1.628583ms, 
2.816639ms]
'/state' response on average took 113.469574ms, 10 responses are in 
[104.218099ms, 134.477062ms]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 3.892615876secs, 10 responses are in 
[2.480517ms, 7.630934838secs]
'/state' response on average took 5.205245306secs, 10 responses are in 
[1.578161651secs, 8.789315237secs]
```

**Batching and `-O3` optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.973573ms, 10 responses are in [1.221193ms, 
2.694713ms]
'/state' response on average took 113.331551ms, 10 responses are in 
[102.593397ms, 142.028555ms]

Test setup: 100

Re: Review Request 68132: Batch '/state' requests on Master.

[Alexander Rukletsov]

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

(Updated July 31, 2018, 5:22 p.m.)


Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

With this patch handlers for '/state' requests are not scheduled
directly after authorization, but are accumulated and then scheduled
for later parallel processing.

This approach allows, if there are N '/state' requests in the Master's
mailbox and T is the request response time, to block the Master actor
only once for time O(T) instead of blocking it for time N*T prior to
this patch.

This batching technique reduces both the time Master is spending
answering '/state' requests and the average request response time
in presence of multiple requests in the Master's mailbox. However,
for seldom '/state' requests that don't accumulate in the Master's
mailbox, the response time might increase due to an added trip
through the mailbox.


Diffs
-

  src/master/http.cpp 6947031da3ce3523408d69d6dac60551a39a4601 
  src/master/master.hpp 0353d550308816f219aedb6afe15c643fc8bb340 
  src/master/master.cpp 2af976f7ea7f81d4b06a45ce13286dbd61b9b144 


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


Testing (updated)
---

`make check` on Mac OS 10.13.5 and various Linux distros.

Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark.

Average improvement without optimization: 62%–70%.
Average improvement with optimization: 17%–62%.

**No batching, no optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.102349605secs, 10 responses are in 
[2.662342ms, 2.143755433secs]
'/state' response on average took 1.549122019secs, 10 responses are in 
[494.278454ms, 2.633971927secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 18.436968137secs, 10 responses are in 
[2.578238ms, 33.210561732secs]
'/state' response on average took 23.916379537secs, 10 responses are in 
[5.170660597secs, 43.008091744secs]
```

**With batching but no optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 417.211022ms, 10 responses are in 
[4.066901ms, 728.045442ms]
'/state' response on average took 830.351291ms, 10 responses are in 
[459.033455ms, 1.208880892secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 5.439950928secs, 10 responses are in 
[3.246906ms, 9.343994388secs]
'/state' response on average took 16.764607823secs, 10 responses are in 
[4.980333091secs, 18.461983916secs]
```

**No batching but `-O3` optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 2.396221ms, 10 responses are in [1.628583ms, 
2.816639ms]
'/state' response on average took 113.469574ms, 10 responses are in 
[104.218099ms, 134.477062ms]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 3.892615876secs, 10 responses are in 
[2.480517ms, 7.630934838secs]
'/state' response on average took 5.205245306secs, 10 responses are in 
[1.578161651secs, 8.789315237secs]
```

**Batching and `-O3` optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.973573ms, 10 responses are in [1.221193ms, 
2.694713ms]
'/state' response on average took 113.331551ms, 10 responses are in 
[102.593397ms, 142.028555ms]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state'

Re: Review Request 67823: Added a master benchmark test for metrics.

[Greg Mann]

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

(Updated July 31, 2018, 5:22 p.m.)


Review request for mesos, Benjamin Mahler, Gastón Kleiman, and James Peach.


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


Repository: mesos


Description
---

Added a master benchmark test for metrics.


Diffs (updated)
-

  src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 


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

Changes: https://reviews.apache.org/r/67823/diff/4-5/


Testing
---


Thanks,

Greg Mann

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Benjamin Bannier]

> On July 31, 2018, 10:27 a.m., Benjamin Bannier wrote:
> > LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using 
> > the original upstream patch would be great if possible.
> 
> Andrew Schwartzmeyer wrote:
> Ben, this did use the oiginal upsteam patch. I cloned the repo, checked 
> out the tag we bundle, and cherry-picked the upstream patch from master, then 
> created a patch file from the applied diff. The commit in master does not 
> apply to 1.8.0 perfectly, so I can't just turn it into a patch, it had to be 
> applied and conflicts fixed. I'm closing the issue again, because this is the 
> original upstream patch, or as close as possible as we can get.
> 
> If you have another way to do it, I'm all ears!

Sorry for possibly being very dense. I did this

# Applied your patch.
mesos [master?] % ./support/apply-reviews.py -r 67916 -c -n -s
# 

# Extracted the _original_ upstream patch.
googletest [master?] % git format-patch 
f66ab00704cd47e4e63ef6d425ca14b9192aaebb~1..f66ab00704cd47e4e63ef6d425ca14b9192aaebb
0001-Upstream-cl-199129756.patch

# Use the _original_ upstream patch in Mesos.
mesos [master?] % mv ~/src/googletest/0001-Upstream-cl-199129756.patch 
3rdparty/googletest-release-1.8.0.patch

# `patch`, at least on macos-10.13.6, is able to automatically apply this 
patch.
mesos [master??] % ninja -C_ 3rdparty/googletest-1.8.0
# 
[1/5] Performing patch step for 'googletest-1.8.0'
patching file googletest/include/gtest/gtest-printers.h
Hunk #1 succeeded at 581 with fuzz 2 (offset -55 lines).
patching file googletest/test/gtest-printers_test.cc
Hunk #1 succeeded at  (offset -4 lines).
# 

Does this not work on e.g., Windows?


- Benjamin


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


On July 30, 2018, 8:50 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> ---
> 
> (Updated July 30, 2018, 8:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
> 
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
> 
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 
>   3rdparty/googletest-release-1.8.0.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67916/diff/2/
> 
> 
> Testing
> ---
> 
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following 
> (temporary) CMake changes:
> 
> ```
>  # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
>  # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
> 
> After adding the necessary compile interface definition, `ninja tests` 
> successfully built on Windows with this patch applied, after not building 
> before the patch.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On July 31, 2018, 4:34 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68111/
> ---
> 
> (Updated July 31, 2018, 4:34 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'MesosCon 2018 CFP is now open!' blog post.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68111/diff/4/
> 
> 
> Testing
> ---
> 
> Used `./mesos-website-dev.sh` to verify that the website is properly rendered.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Andrew Schwartzmeyer]

> On July 31, 2018, 1:27 a.m., Benjamin Bannier wrote:
> > LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using 
> > the original upstream patch would be great if possible.

Ben, this did use the oiginal upsteam patch. I cloned the repo, checked out the 
tag we bundle, and cherry-picked the upstream patch from master, then created a 
patch file from the applied diff. The commit in master does not apply to 1.8.0 
perfectly, so I can't just turn it into a patch, it had to be applied and 
conflicts fixed. I'm closing the issue again, because this is the original 
upstream patch, or as close as possible as we can get.

If you have another way to do it, I'm all ears!


- Andrew


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


On July 30, 2018, 11:50 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> ---
> 
> (Updated July 30, 2018, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
> 
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
> 
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 
>   3rdparty/googletest-release-1.8.0.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67916/diff/2/
> 
> 
> Testing
> ---
> 
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following 
> (temporary) CMake changes:
> 
> ```
>  # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
>  # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
> 
> After adding the necessary compile interface definition, `ninja tests` 
> successfully built on Windows with this patch applied, after not building 
> before the patch.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.

[Gastón Kleiman]

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

(Updated July 31, 2018, 9:34 a.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Fixed typos, made the text moar formal.


Repository: mesos


Description
---

Added 'MesosCon 2018 CFP is now open!' blog post.


Diffs (updated)
-

  site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION 


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

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


Testing
---

Used `./mesos-website-dev.sh` to verify that the website is properly rendered.


Thanks,

Gastón Kleiman

Re: Review Request 68132: Batch '/state' requests on Master.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68132 was successfully built and tested.

Reviews applied: `['68131', '68132']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2011/mesos-review-68132

- Mesos Reviewbot Windows


On July 31, 2018, 3:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68132/
> ---
> 
> (Updated July 31, 2018, 3:05 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9122
> https://issues.apache.org/jira/browse/MESOS-9122
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this patch handlers for '/state' requests are not scheduled
> directly after authorization, but are accumulated and then scheduled
> for later parallel processing.
> 
> This approach allows, if there are N '/state' requests in the Master's
> mailbox and T is the request response time, to block the Master actor
> only once for time O(T) instead of blocking it for time N*T prior to
> this patch.
> 
> This batching technique reduces both the time Master is spending
> answering '/state' requests and the average request response time
> in presence of multiple requests in the Master's mailbox. However,
> for seldom '/state' requests that don't accumulate in the Master's
> mailbox, the response time might increase due to an added trip
> through the mailbox.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6947031da3ce3523408d69d6dac60551a39a4601 
>   src/master/master.hpp 0353d550308816f219aedb6afe15c643fc8bb340 
>   src/master/master.cpp 2af976f7ea7f81d4b06a45ce13286dbd61b9b144 
> 
> 
> Diff: https://reviews.apache.org/r/68132/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.13.5 and various Linux distros.
> 
> Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark.
> 
> **Without this patch**
> ```
> Test setup: 100 agents with a total of 1 running tasks and 1 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 1.102349605secs, 10 responses are in 
> [2.662342ms, 2.143755433secs]
> '/state' response on average took 1.549122019secs, 10 responses are in 
> [494.278454ms, 2.633971927secs]
> 
> Test setup: 1000 agents with a total of 10 running tasks and 10 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 18.436968137secs, 10 responses are in 
> [2.578238ms, 33.210561732secs]
> '/state' response on average took 23.916379537secs, 10 responses are in 
> [5.170660597secs, 43.008091744secs]
> ```
> 
> **With this patch**
> ```
> Test setup: 100 agents with a total of 1 running tasks and 1 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 417.211022ms, 10 responses are in 
> [4.066901ms, 728.045442ms]
> '/state' response on average took 830.351291ms, 10 responses are in 
> [459.033455ms, 1.208880892secs]
> 
> Test setup: 1000 agents with a total of 10 running tasks and 10 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 5.439950928secs, 10 responses are in 
> [3.246906ms, 9.343994388secs]
> '/state' response on average took 16.764607823secs, 10 responses are in 
> [4.980333091secs, 18.461983916secs]
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68132: Batch '/state' requests on Master.

[Alexander Rukletsov]

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

(Updated July 31, 2018, 3:05 p.m.)


Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

With this patch handlers for '/state' requests are not scheduled
directly after authorization, but are accumulated and then scheduled
for later parallel processing.

This approach allows, if there are N '/state' requests in the Master's
mailbox and T is the request response time, to block the Master actor
only once for time O(T) instead of blocking it for time N*T prior to
this patch.

This batching technique reduces both the time Master is spending
answering '/state' requests and the average request response time
in presence of multiple requests in the Master's mailbox. However,
for seldom '/state' requests that don't accumulate in the Master's
mailbox, the response time might increase due to an added trip
through the mailbox.


Diffs
-

  src/master/http.cpp 6947031da3ce3523408d69d6dac60551a39a4601 
  src/master/master.hpp 0353d550308816f219aedb6afe15c643fc8bb340 
  src/master/master.cpp 2af976f7ea7f81d4b06a45ce13286dbd61b9b144 


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


Testing
---

`make check` on Mac OS 10.13.5 and various Linux distros.

Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark.

**Without this patch**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.102349605secs, 10 responses are in 
[2.662342ms, 2.143755433secs]
'/state' response on average took 1.549122019secs, 10 responses are in 
[494.278454ms, 2.633971927secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 18.436968137secs, 10 responses are in 
[2.578238ms, 33.210561732secs]
'/state' response on average took 23.916379537secs, 10 responses are in 
[5.170660597secs, 43.008091744secs]
```

**With this patch**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 417.211022ms, 10 responses are in 
[4.066901ms, 728.045442ms]
'/state' response on average took 830.351291ms, 10 responses are in 
[459.033455ms, 1.208880892secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 5.439950928secs, 10 responses are in 
[3.246906ms, 9.343994388secs]
'/state' response on average took 16.764607823secs, 10 responses are in 
[4.980333091secs, 18.461983916secs]
```


Thanks,

Alexander Rukletsov

Review Request 68132: Batch '/state' requests on Master.

[Alexander Rukletsov]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

With this patch handlers for '/state' requests are not scheduled
directly after authorization, but are accumulated and then scheduled
for later parallel processing.

This approach allows, if there are N '/state' requests in the Master's
mailbox and T is the request response time, to block the Master actor
only once for time O(T) instead of blocking it for time N*T prior to
this patch.

This batching technique reduces both the time Master is spending
answering '/state' requests and the average request responce time
in presence of multiple requests in the Master's mailbox. However,
for seldom '/state' requests that don't accumulate in the Master's
mailbox, the response time might increase due to an added trip
through the mailbox.


Diffs
-

  src/master/http.cpp 6947031da3ce3523408d69d6dac60551a39a4601 
  src/master/master.hpp 0353d550308816f219aedb6afe15c643fc8bb340 
  src/master/master.cpp 2af976f7ea7f81d4b06a45ce13286dbd61b9b144 


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


Testing
---

`make check` on Mac OS 10.13.5 and various Linux distros.

Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark.

**Without this patch**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.102349605secs, 10 responses are in 
[2.662342ms, 2.143755433secs]
'/state' response on average took 1.549122019secs, 10 responses are in 
[494.278454ms, 2.633971927secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 18.436968137secs, 10 responses are in 
[2.578238ms, 33.210561732secs]
'/state' response on average took 23.916379537secs, 10 responses are in 
[5.170660597secs, 43.008091744secs]
```

**With this patch**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 417.211022ms, 10 responses are in 
[4.066901ms, 728.045442ms]
'/state' response on average took 830.351291ms, 10 responses are in 
[459.033455ms, 1.208880892secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 5.439950928secs, 10 responses are in 
[3.246906ms, 9.343994388secs]
'/state' response on average took 16.764607823secs, 10 responses are in 
[4.980333091secs, 18.461983916secs]
```


Thanks,

Alexander Rukletsov

Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Alexander Rukletsov]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 


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


Testing
---

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


Thanks,

Alexander Rukletsov

Re: Review Request 68120: UI: Pull up the leader URL generation to a top-level function.

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On July 31, 2018, 2:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68120/
> ---
> 
> (Updated July 31, 2018, 2:40 a.m.)
> 
> 
> Review request for mesos, Armand Grillet and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This path pulls up the leader URL prefix generation to a top-level
> function in order to have it re-used across controllers.
> 
> Having this logic in just one place makes it easier to modify it,
> e.g., to make it possible to use the UI via a reverse proxy.
> 
> 
> Diffs
> -
> 
>   src/webui/app/controllers.js 7c228a1629d7b1dcce56b432f042f02d3ec1583b 
>   src/webui/app/home.html ff965952d7a8d84c9dcfd5d6e7e045cd72bc 
> 
> 
> Diff: https://reviews.apache.org/r/68120/diff/1/
> 
> 
> Testing
> ---
> 
> manual testing
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68120: UI: Pull up the leader URL generation to a top-level function.

[Armand Grillet]

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




src/webui/app/controllers.js
Lines 515 (patched)


This is the first time we use a ternary operator in this file but we use 
some in `app.js` and it makes sense to use it here.


- Armand Grillet


On July 31, 2018, 2:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68120/
> ---
> 
> (Updated July 31, 2018, 2:40 a.m.)
> 
> 
> Review request for mesos, Armand Grillet and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This path pulls up the leader URL prefix generation to a top-level
> function in order to have it re-used across controllers.
> 
> Having this logic in just one place makes it easier to modify it,
> e.g., to make it possible to use the UI via a reverse proxy.
> 
> 
> Diffs
> -
> 
>   src/webui/app/controllers.js 7c228a1629d7b1dcce56b432f042f02d3ec1583b 
>   src/webui/app/home.html ff965952d7a8d84c9dcfd5d6e7e045cd72bc 
> 
> 
> Diff: https://reviews.apache.org/r/68120/diff/1/
> 
> 
> Testing
> ---
> 
> manual testing
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['68103', '68104']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2010/mesos-review-68104

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2010/mesos-review-68104/logs/mesos-tests-stdout.log):

```
[--] 9 tests from Endpoint/SlaveEndpointTest (929 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (37 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (72 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (625 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (648 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (680 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (706 ms total)

[--] Global test environment tear-down
[==] 1013 tests from 98 test cases ran. (511266 ms total)
[  PASSED  ] 1011 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/0, where GetParam() 
= application/x-protobuf
[  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/1, where GetParam() 
= application/json

 2 FAILED TESTS
  YOU HAVE 222 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2010/mesos-review-68104/logs/mesos-tests-stderr.log):

```
I0731 11:21:49.415030 50408 master.cpp:10917] Updating the state of task 
007f0173-d0d3-4f8a-a6ae-5c6b17b727a8 of framework 
1b290b3b-4469-448d-8fca-f9840778b748- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0731 11:21:49.415030 44552 slave.cpp:3939] Shutting down framework 
1b290b3b-4469-448d-8fca-f9840778b748-
I0731 11:21:49.415693 44552 slave.cpp:6658] Shutting down executor 
'007f0173-d0d3-4f8a-a6ae-5c6b17b727a8' of framework 
1b290b3b-4469-448d-8fca-f9840778b748- at executor(1)@192.10.1.6:49239
I0731 11:21:49.417690 44552 slave.cpp:931] Agent terminating
W0731 11:21:49.417690 44552 slave.cpp:3935] Ignoring shutdown framework 
1b290b3b-4469-448d-8fca-f9840778b748- because it is terminating
I0731 11:21:49.417690 50408 master.cpp:11016] Removing task 
007f0173-d0d3-4f8a-a6ae-5c6b17b727a8 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 1b290b3b-4469-448d-8fca-f9840778b748- on 
agent 1I0731 11:21:49.247696 48276 exec.cpp:162] Version: 1.7.0
I0731 11:21:49.273716 50344 exec.cpp:236] Executor registered on agent 
1b290b3b-4469-448d-8fca-f9840778b748-S0
I0731 11:21:49.276705 50884 executor.cpp:182] Received SUBSCRIBED event
I0731 11:21:49.281693 50884 executor.cpp:186] Subscribed executor on 
windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net
I0731 11:21:49.281693 50884 executor.cpp:182] Received LAUNCH event
I0731 11:21:49.286708 50884 executor.cpp:679] Starting task 
007f0173-d0d3-4f8a-a6ae-5c6b17b727a8
I0731 11:21:49.368700 50884 executor.cpp:499] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0731 11:21:49.376683 50884 executor.cpp:693] Forked command at 46460
I0731 11:21:49.417690 50276 exec.cpp:445] Executor asked to shutdown
I0731 11:21:49.418694 50884 executor.cpp:182] Received SHUTDOWN event
I0731 11:21:49.418694 50884 executor.cpp:796] Shutting down
I0731 11:21:49.418694 50884 executor.cpp:909] Sending SIGTERM to process tree 
at pid 46b290b3b-4469-448d-8fca-f9840778b748-S0 at slave(462)@192.10.1.6:63931 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0731 11:21:49.421685 50408 master.cpp:1330] Agent 
1b290b3b-4469-448d-8fca-f9840778b748-S0 at slave(462)@192.10.1.6:63931 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net) disconnected
I0731 11:21:49.421685 50408 master.cpp:3340] Disconnecting agent 
1b290b3b-4469-448d-8fca-f9840778b748-S0 at slave(462)@192.10.1.6:63931 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0731 11:21:49.421685 49432 hierarchical.cpp:345] Removed framewor

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

[Benjamin Bannier]

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

(Updated July 31, 2018, 12:19 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Addressed comments from Chun.


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


Repository: mesos


Description
---

Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.


Diffs (updated)
-

  src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
  src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

[Benjamin Bannier]

> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1818 (patched)
> > 
> >
> > Conventionally we only indent this by 4 extra spaces aligning with 
> > "return".

Good catch, I filed https://issues.apache.org/jira/browse/MESOS-9121.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1821-1822 (patched)
> > 
> >
> > The following can be fit into 80 characters:
> > ```
> > [this, acceptType](const Owned& approvers) -> 
> > Response {
> > ```
> > Or for here it seems harmless to just use `[=]`. I'm fine with either 
> > though.

That line seems to be exactly 81 characters long, so it looks like we do need 
to split somewhere. I now manually adjusted this to not split the capture list 
(the previous version was formated by our `clang-format`).

I don't think that implicitly capturing pointers to not immediately invoked 
lambdas is safe style, much less when capturing `this` like here. I'll keep the 
explicit capture.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1824 (patched)
> > 
> >
> > Should we return an empty list of resource provider infos or return a 
> > 403 Forbidden?

Good point, changed to return an empty set when not authorized. This maps well 
on e.g., specifying sets of RPs somebody can view in the local authorizer.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 7002-7004 (original), 7002-7021 (patched)
> > 
> >
> > How about the following:
> > ```
> > slave::Flags slaveFlags = CreateSlaveFlags();
> > slaveFlags.authenticate_http_readwrite = true;
> > 
> > {
> >   // `DEFAULT_CREDENTIAL_2` is not allowed to view any resource 
> > provider.
> >   mesos::ACL::ViewResourceProvider* acl =
> > slaveFlags.acls->add_view_resource_providers();
> >   
> > acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL_2.principal());
> >   acl->mutable_resource_providers()->set_type(mesos::ACL::Entity::NONE);
> > {
> > 
> > Try> slave = StartSlave(&detector, slaveFlags);
> > ```

Great suggestion, applied.

I did not set `authenticate_http_readwrite` though since it is not required.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 7046-7049 (patched)
> > 
> >
> > I'm a bit against checking content of the failure string since it's 
> > slightly hard to maintain. Can we avoid this?

I removed this check for now as knowning that the request failed already tell 
us _something_.


- Benjamin


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


On July 31, 2018, 12:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated July 31, 2018, 12:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8314
> https://issues.apache.org/jira/browse/MESOS-8314
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
>   src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 
> 
> 
> Diff: https://reviews.apache.org/r/68104/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68103: Added an authorizer action for viewing of resource provider information.

[Benjamin Bannier]

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

(Updated July 31, 2018, 12:19 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Addressed comments from Chun.


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


Repository: mesos


Description
---

Added an authorizer action for viewing of resource provider information.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto 1777c04316bfd7493894f0e782f170fe8437aafe 
  include/mesos/authorizer/authorizer.proto 
8b5fa09f389ca4c26f8390d35af32c4cdd561418 
  src/authorizer/local/authorizer.cpp abf5b4663cd517fb6d69b5373dd0e6520e87cf8e 
  src/tests/authorization_tests.cpp 41ecac29a53f6ad9553cbf0a1121e1f3cff50df8 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 68127: Add `ROOT_INTERNET_CURL_UNPRIVILEGED_USER_CommandTaskRootfsWithVolume`.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66811.

Failed command: `python.exe .\support\python3\apply-reviews.py -n -r 66811`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2009/mesos-review-68127

Relevant logs:

- 
[apply-review-66811-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2009/mesos-review-68127/logs/apply-review-66811-stdout.log):

```
error: patch failed: 3rdparty/stout/CMakeLists.txt:30
error: 3rdparty/stout/CMakeLists.txt: patch does not apply
```

- Mesos Reviewbot Windows


On July 31, 2018, 8:35 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68127/
> ---
> 
> (Updated July 31, 2018, 8:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8812
> https://issues.apache.org/jira/browse/MESOS-8812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `ROOT_INTERNET_CURL_UNPRIVILEGED_USER_CommandTaskRootfsWithVolume`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c15a6fad642474765e4ad1952af6cd9ee937379e 
> 
> 
> Diff: https://reviews.apache.org/r/68127/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 68127: Add `ROOT_INTERNET_CURL_UNPRIVILEGED_USER_CommandTaskRootfsWithVolume`.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Add `ROOT_INTERNET_CURL_UNPRIVILEGED_USER_CommandTaskRootfsWithVolume`.


Diffs
-

  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c15a6fad642474765e4ad1952af6cd9ee937379e 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 68126: Added a test `ROOT_UNPRIVILEGED_USER_CommandTaskNoRootfsWithVolume`.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Added a test `ROOT_UNPRIVILEGED_USER_CommandTaskNoRootfsWithVolume`.


Diffs
-

  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c15a6fad642474765e4ad1952af6cd9ee937379e 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68125: Granted container user permissions for DOCKER_VOLUME volume.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Granted container user permissions for DOCKER_VOLUME volume.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
76f1a5243c8d5028157f795d851b547a5ce57ac9 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
ab749be6234a5eedc0617a131c126129f43f8d62 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Benjamin Bannier]

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


Ship it!




LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using the 
original upstream patch would be great if possible.

- Benjamin Bannier


On July 30, 2018, 8:50 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> ---
> 
> (Updated July 30, 2018, 8:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
> 
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
> 
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 
>   3rdparty/googletest-release-1.8.0.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67916/diff/2/
> 
> 
> Testing
> ---
> 
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following 
> (temporary) CMake changes:
> 
> ```
>  # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
>  # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
> 
> After adding the necessary compile interface definition, `ninja tests` 
> successfully built on Windows with this patch applied, after not building 
> before the patch.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68094: Add port mapping and network ports isolators sources to CMake.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On July 27, 2018, 9:11 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68094/
> ---
> 
> (Updated July 27, 2018, 9:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, James Peach, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8993 and MESOS-8994
> https://issues.apache.org/jira/browse/MESOS-8993
> https://issues.apache.org/jira/browse/MESOS-8994
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the relevant options and source files, but with a fatal
> message if they're used, as we do not yet check for their
> dependencies. This also adds the `mesos-network-helper` executable as
> used by the port mapping isolator.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake f80c265d117c70acda95da6b943cdd3e4e477daa 
>   src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
>   src/slave/containerizer/mesos/CMakeLists.txt 
> ba1f92fe7dd59c34c6dee0bc7ecf6f1b5160eee8 
>   src/tests/CMakeLists.txt 695b6f543847d4270f7004a1768919904975d7a4 
> 
> 
> Diff: https://reviews.apache.org/r/68094/diff/4/
> 
> 
> Testing
> ---
> 
> Pending... There isn't much to test for the isolators added, as they're added 
> with a fatal bailout. Double-checking that without enabling the new options, 
> everything still works as expected.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.

[Benjamin Bannier]

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


Fix it, then Ship it!





site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md
Lines 16 (patched)


`s/We're/We are/`



site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md
Lines 32 (patched)


`s/you/your/`


- Benjamin Bannier


On July 30, 2018, 10:38 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68111/
> ---
> 
> (Updated July 30, 2018, 10:38 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'MesosCon 2018 CFP is now open!' blog post.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68111/diff/3/
> 
> 
> Testing
> ---
> 
> Used `./mesos-website-dev.sh` to verify that the website is properly rendered.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 68123: Avoided unnecessary `Resources::allocations()` call in the allocator.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68123 was successfully built and tested.

Reviews applied: `['68118', '68119', '68122', '68123']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2008/mesos-review-68123

- Mesos Reviewbot Windows


On July 31, 2018, 5:17 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68123/
> ---
> 
> (Updated July 31, 2018, 5:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.

[Liangyu Zhao via Review Board]

> On July 30, 2018, 11:39 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 1016-1017 (patched)
> > 
> >
> > Are we just skipping a failed blob here and trying to process the rest? 
> > Does that make sense to do?

Yes, the code will try each url until one succeeds. If all fail, the fetch will 
be considered failed.


- Liangyu


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


On July 30, 2018, 10:40 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated July 30, 2018, 10:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
>   src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>