Re: Review Request 61575: Added a test for verifying signal escalation on the default executor.

[Gastón Kleiman]

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




src/tests/default_executor_tests.cpp
Lines 599 (patched)


Please use `ASSERT_FALSE(offers->offers().empty())` instead.



src/tests/default_executor_tests.cpp
Lines 609 (patched)


Remove the empty line.



src/tests/default_executor_tests.cpp
Lines 621 (patched)


I think we should make the grace period a bit longer to be able to check if 
a `TASK_KILLING` status update is sent.



src/tests/default_executor_tests.cpp
Lines 625-626 (patched)


I think that it'd be more readable to remove this from here, and to use the 
following instead of the `CALL` block that follows:

```
  v1::Offer::Operation launchGroup = v1::LAUNCH_GROUP(
  executorInfo,
  v1::createTaskGroupInfo({task1, task2}));

  mesos.send(v1::createCallAccept(frameworkId, offer, {launchGroup}));
```



src/tests/default_executor_tests.cpp
Lines 629-630 (patched)


If you  use the following, you can remove the acknowledge block bellow:

```
  EXPECT_CALL(*scheduler, update(_, _))
.WillOnce(DoAll(
FutureArg<1>(),
v1::scheduler::SendAcknowledge(
frameworkId,
offer.agent_id(;
```



src/tests/default_executor_tests.cpp
Lines 675 (patched)


s/signify/signal/



src/tests/default_executor_tests.cpp
Lines 721 (patched)


This checks that the process finally got a `SIGKILL`, but we can't be sure 
if it got a `SIGTERM` before that.

I couldn't find a test that checks this with the other executors, but I 
think that we should at least add a TODO.


- Gastón Kleiman


On Aug. 15, 2017, 10:11 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61575/
> ---
> 
> (Updated Aug. 15, 2017, 10:11 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This also required a small modification to the kill test helper binary
> to write a marker file signifying that the signal handlers have been
> set up correctly.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> b9776314a8781963b92ba9ac297654f61a443bc8 
>   src/tests/kill_policy_test_helper.hpp 
> 29651102ec46b477e6e797c6e6bdef5b10afa665 
>   src/tests/kill_policy_test_helper.cpp 
> a1880595ff015475f1ba49437d49f7397da19422 
> 
> 
> Diff: https://reviews.apache.org/r/61575/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61573: Made the default executor support signal escalation.

[Anand Mazumdar]

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

(Updated Aug. 16, 2017, 12:21 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description (updated)
---

This modifies the default executor to perform signal escalation
via the 'KILL_NESTED_CONTAINER' call i.e., send a SIGTERM followed
by a SIGKILL after some grace period. Note that support for kill
policies still needs to be done instead of using a constant grace
period.

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


Diffs (updated)
-

  src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 


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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 61675: Updated doc for the changes of pid namespace sharing.

[Qian Zhang]

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


Fix it, then Ship it!





docs/mesos-containerizer.md
Lines 79 (patched)


s/access/share/ to be consistent with the name of the agent flag.



docs/mesos-containerizer.md
Lines 82 (patched)


s/'/`


- Qian Zhang


On Aug. 16, 2017, 6:50 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61675/
> ---
> 
> (Updated Aug. 16, 2017, 6:50 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, Greg Mann, Jie Yu, 
> Qian Zhang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated doc for the changes of pid namespace sharing.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md feaaa556c9646435c6c9d677569962576ebf4c74 
> 
> 
> Diff: https://reviews.apache.org/r/61675/diff/1/
> 
> 
> Testing
> ---
> 
> N/A.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61675: Updated doc for the changes of pid namespace sharing.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61675]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 15, 2017, 10:50 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61675/
> ---
> 
> (Updated Aug. 15, 2017, 10:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, Greg Mann, Jie Yu, 
> Qian Zhang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated doc for the changes of pid namespace sharing.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md feaaa556c9646435c6c9d677569962576ebf4c74 
> 
> 
> Diff: https://reviews.apache.org/r/61675/diff/1/
> 
> 
> Testing
> ---
> 
> N/A.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 61675: Updated doc for the changes of pid namespace sharing.

[Gilbert Song]

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

Review request for mesos, Anand Mazumdar, Gastón Kleiman, Greg Mann, Jie Yu, 
Qian Zhang, and Vinod Kone.


Repository: mesos


Description
---

Updated doc for the changes of pid namespace sharing.


Diffs
-

  docs/mesos-containerizer.md feaaa556c9646435c6c9d677569962576ebf4c74 


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


Testing
---

N/A.


Thanks,

Gilbert Song

Re: Review Request 61575: Added a test for verifying signal escalation on the default executor.

[Anand Mazumdar]

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

(Updated Aug. 15, 2017, 10:11 p.m.)


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


Changes
---

Review comments.


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


Repository: mesos


Description (updated)
---

This also required a small modification to the kill test helper binary
to write a marker file signifying that the signal handlers have been
set up correctly.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 
  src/tests/kill_policy_test_helper.hpp 
29651102ec46b477e6e797c6e6bdef5b10afa665 
  src/tests/kill_policy_test_helper.cpp 
a1880595ff015475f1ba49437d49f7397da19422 


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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 61664: Libprocess: Added a timeout for send socket operation.

[Vinod Kone]

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




3rdparty/libprocess/src/process.cpp
Lines 256-261 (patched)


How did you pick these values?



3rdparty/libprocess/src/process.cpp
Lines 2184 (patched)


s/, see/; see/



3rdparty/libprocess/src/process.cpp
Lines 2170-2182 (original), 2195-2209 (patched)


I guess this means if writing a very large message takes greater than the 
timeout we abort? Do you have a rought back of the envelope calculation for 
what the max data size could be given the minimum timeout?


- Vinod Kone


On Aug. 15, 2017, 3:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61664/
> ---
> 
> (Updated Aug. 15, 2017, 3:45 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7748
> https://issues.apache.org/jira/browse/MESOS-7748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, a send socket operation can wait forever for
> a send to complete. Clients that drop connections or stop reading
> incoming data, aka "slow reader" attack, can eventually exhaust the
> resources of a libprocess-based application and cause denial of
> service or an OOM event.
> 
> This patch introduces an obligatory timeout for all send socket
> operations, after which the stalled connection is dropped. The
> timeout can be adjusted via the `LIBPROCESS_SOCKET_SEND_TIMEOUT`
> environment variable.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> dcd9c6738816764aae066fe56cd5f468c98fc9bd 
> 
> 
> Diff: https://reviews.apache.org/r/61664/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing with a rogue client.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61668: Added support for kill policies to the default executor.

[Anand Mazumdar]

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

(Updated Aug. 15, 2017, 8:37 p.m.)


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


Changes
---

Review comments.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 


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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 61574: Added `kill()` support to the test containerizer interface.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 15, 2017, 5:41 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61574/
> ---
> 
> (Updated Aug. 15, 2017, 5:41 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is needed to make some tests pass that rely on the test
> containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.hpp 4bd40c32625bc1f7998d523c6ee81bf78ac74538 
>   src/tests/containerizer.cpp 1d2b6391cf7a7545fa44206c59d05764f3e8cdfb 
> 
> 
> Diff: https://reviews.apache.org/r/61574/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61668: Added support for kill policies to the default executor.

[Vinod Kone]

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




src/launcher/default_executor.cpp
Line 1098 (original), 1122-1130 (patched)


I would move this to `kill()` so that the update can be sent in other kill 
paths as well (e.g., when one task in a group terminates and all others are 
killed by the executor)


- Vinod Kone


On Aug. 15, 2017, 6:59 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61668/
> ---
> 
> (Updated Aug. 15, 2017, 6:59 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6535
> https://issues.apache.org/jira/browse/MESOS-6535
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/61668
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61668/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 60620: Modify os::write to write binary files on Windows.

[Andrew Schwartzmeyer]

> On Aug. 15, 2017, 10:47 a.m., Andrew Schwartzmeyer wrote:
> > Ship It!

You can remove the "depends on 60345" here to make the bot a bit happier.


- Andrew


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


On July 5, 2017, 10:43 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60620/
> ---
> 
> (Updated July 5, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modify os::write to write binary files on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/write.hpp 
> beb5bd83b52565a75e34d32b5bb17951bc799578 
> 
> 
> Diff: https://reviews.apache.org/r/60620/diff/2/
> 
> 
> Testing
> ---
> 
> Built successfully on both Linux (with autotools and cmake) and Windows (with 
> cmake).
> 
> Ran stout-tests and mesos-tests successfully on both Windows and Linux.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61573: Made the default executor support signal escalation.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 14, 2017, 5:19 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61573/
> ---
> 
> (Updated Aug. 14, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This modifies the default executor to perform signal escalation
> via the 'KILL_NESTED_CONTAINER' call i.e., send a SIGTERM followed
> by a SIGKILL after some grace period. Note that support for kill
> policies still needs to be done instead of using a constant grace
> period.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61573/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61570: Added the `kill()` function to the containerizer interface.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 14, 2017, 11:09 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61570/
> ---
> 
> (Updated Aug. 14, 2017, 11:09 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would be used by now on for killing a container by sending
> a signal to it similar to the linux equivalent `kill()` system call.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0954ed6175e4c8f963bf371e54e0f9ffe7bc9c1c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> fd586306f7a6d3d2cdb58481f0ef4906de8d0f88 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ff192bb085f3554ad1b1f20fb73bf827bf04ef13 
> 
> 
> Diff: https://reviews.apache.org/r/61570/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61668: Added support for kill policies to the default executor.

[Anand Mazumdar]

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

(Updated Aug. 15, 2017, 6:59 p.m.)


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


Changes
---

Review comments.


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


Repository: mesos


Description (updated)
---

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


Diffs (updated)
-

  src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 


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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 61668: Added support for kill policies to the default executor.

[Anand Mazumdar]

> On Aug. 15, 2017, 6:09 p.m., Gastón Kleiman wrote:
> > src/launcher/default_executor.cpp
> > Line 995 (original), 1001 (patched)
> > 
> >
> > We should send a TASK_KILLING update here, and address the TODOs added 
> > by AlexR.

Good catch! I had missed that since I sent out this patch before updating the 
tests.


- Anand


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


On Aug. 15, 2017, 5:40 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61668/
> ---
> 
> (Updated Aug. 15, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6535
> https://issues.apache.org/jira/browse/MESOS-6535
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61668/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 60628: Enable fetcher_tests.cpp unit test module on Windows platform.

[Andrew Schwartzmeyer]

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




src/launcher/CMakeLists.txt
Lines 38-65 (original), 38-57 (patched)


I rewrote this file. Poor Joe :(



src/slave/containerizer/fetcher.cpp
Line 780 (original), 778-783 (patched)


I don't think this change is necessary. On Windows, when this eventually 
reaches `CreateProcess`, it tries the various suffixes such as `.exe` to launch 
the process.

Furthermore, we don't do this anywhere else, so it's inconsistent.



src/tests/fetcher_tests.cpp
Line 355 (original), 359 (patched)


Ah. Precisely one use of this odd boolean flag to `path::uri`. I would 
definitely suggest this just be `path::normalize`. Or make `path::uri` 
idempotent (i.e. only add the prefix if it's not already there, if that's the 
logic you're needing here.).



src/tests/fetcher_tests.cpp
Line 466 (original), 470 (patched)


Huh?



src/tests/fetcher_tests.cpp
Line 512 (original), 516 (patched)


Another, huh?



src/tests/fetcher_tests.cpp
Line 565 (original), 569 (patched)


This looks like it dropped the `localhost` part.



src/tests/fetcher_tests.cpp
Line 593 (original), 598 (patched)


Good place for `path::normalize`.



src/tests/fetcher_tests.cpp
Line 739 (original), 754 (patched)


Ditto.



src/tests/fetcher_tests.cpp
Line 827 (original), 845 (patched)


Ditto.



src/tests/fetcher_tests.cpp
Line 880 (original), 898 (patched)


Ditto.



src/tests/fetcher_tests.cpp
Line 937 (original), 969 (patched)


Ditto.



src/tests/fetcher_tests.cpp
Line 979 (original), 1011 (patched)


Ditto.


- Andrew Schwartzmeyer


On July 3, 2017, 12:35 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60628/
> ---
> 
> (Updated July 3, 2017, 12:35 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document that some FetcherTest tests won't work on Windows platform.
>   Tests for tar, gzip, and such won't be working on Windows for
>   the time being. Thoughts are to provide this capability to the
>   Fetcher in a cross-platform manner via a programmatic code library
>   rather than Linux-specific command line tools (tar, gzip, etc).
> 
>   In the short term, zip and unzip will work since PowerShell can
>   support that natively.
> 
> Fix FetcherTest.AbsoluteCustomSubdirectoryFails on Windows platform.
> Test FetcherTest.InvalidUser can't work on Windows for now.
> Fix test FetcherTest.NonExistingFile on Windows platform.
> Fix test FetcherTest.AbsolutePath on Windows platform.
> Fix test FetcherTest.RelativeFilePath on Windows platform.
> Fix test FetcherTest.OSNetUriTest on Windows platform.
> Fix test FetcherTest.OSNetUriSpaceTest on Windows platform.
> Fix test FetcherTest.FileLocalhostURI on Windows platform.
> Fix test FetcherTest.Unzip_ExtractFile on Windows platform.
> Fix test FetcherTest.Unzip_ExtractInvalidFile on Windows platform.
> Fix test Unzip_ExtractFileWithDuplicatedEntries on Windows platform.
> Fix test FetcherTest.UseCustomOutputFile on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/launcher/CMakeLists.txt 97edc4a28a995da8a5e963aaa62c4c955cc59719 
>   src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
>   src/slave/containerizer/fetcher.cpp 
> e3c786b36ad16b33ef9d3eed15f722890e80f0bb 
>   src/tests/fetcher_tests.cpp 99149baa1c7abfabf572a0d0f4512a8e84d1e5be 
> 
> 
> Diff: https://reviews.apache.org/r/60628/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61262: Added 'heartbeat' event for the operator API.

[Greg Mann]

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




src/master/master.hpp
Lines 309-311 (patched)


Now that we're sending heartbeats for multiple purposes, I'm concerned that 
the logging will be ambiguous. Instead of templating this class on the ID type, 
perhaps it makes more sense to initialize it with a string that it can use for 
logging purposes. For example:

```
class Heartbeater : public process::Process
{
public:
  Heartbeater(const string& _message,
  const HttpConnection& _http,
  const Duration& _interval,
  const Duration& _delay = Seconds(0))
: process::ProcessBase(process::ID::generate("heartbeater")),
  message(_message),
  http(_http),
  interval(_interval),
  delay(_delay) {}

...

private:
  void heartbeat()
  {
// Only send a heartbeat if the connection is not closed.
if (http.closed().isPending()) {
  VLOG(1) << "Sending heartbeat to " << message;
  
  etc...
```

and then for the framework case we would do something like:
```
heartbeater = new Heartbeater(
"framework " + stringify(info.id()),
http.get(),
DEFAULT_HEARTBEAT_INTERVAL);
```

and the subscriber case something like:
```
heartbeater = new Heartbeater(
"subscriber " + stringify(http.streamId),
http,
DEFAULT_HEARTBEAT_INTERVAL,
DEFAULT_HEARTBEAT_INTERVAL);
```

what do you think?



src/tests/api_tests.cpp
Lines 2381 (patched)


How about:
"Verifies that 'HEARTBEAT' events are sent at the correct times."



src/tests/api_tests.cpp
Lines 2444 (patched)


You should also verify that the type of the event is correct here.


- Greg Mann


On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> ---
> 
> (Updated Aug. 14, 2017, 9:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b 
>   include/mesos/v1/master/master.proto 
> c3fb31de2509adcdec8204f8bbe46b46a31540e8 
>   src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 
>   src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 
>   src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f 
> 
> 
> Diff: https://reviews.apache.org/r/61262/diff/5/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61666: Added a test to verify filtering of resource reservations on agent.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60915, 61666]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 15, 2017, 5:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61666/
> ---
> 
> (Updated Aug. 15, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7892
> https://issues.apache.org/jira/browse/MESOS-7892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends `SlaveAuthorizerTest.FilterStateEndpoint` test to
> check that `/state` endpoint shows resource reservations only for
> allowed principal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp 
> 536a8efda0cc1ba08285c379b01af6ec64a82117 
> 
> 
> Diff: https://reviews.apache.org/r/61666/diff/1/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61483: Added a test using CMD health checks + DefaultExecutor w/ Docker image.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61483, 61438, 61282]

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

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
  File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
  File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 
25: ordinal not in range(128)

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/231/console

- Mesos Reviewbot Windows


On Aug. 7, 2017, 10:47 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61483/
> ---
> 
> (Updated Aug. 7, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test using CMD health checks + DefaultExecutor w/ Docker image.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
> 
> 
> Diff: https://reviews.apache.org/r/61483/diff/1/
> 
> 
> Testing
> ---
> 
> `bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.DefaultExecutorWithDockerImageCommandHealthCheck"`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61483: Added a test using CMD health checks + DefaultExecutor w/ Docker image.

[Alexander Rukletsov]

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


Ship it!





src/tests/health_check_tests.cpp
Lines 2458 (patched)


This is how one can still leave traces in the codebase long after 
abandoning the project : )


- Alexander Rukletsov


On Aug. 7, 2017, 10:47 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61483/
> ---
> 
> (Updated Aug. 7, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test using CMD health checks + DefaultExecutor w/ Docker image.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp e824d1b2c2b17423394da0b089ae8940450378b7 
> 
> 
> Diff: https://reviews.apache.org/r/61483/diff/1/
> 
> 
> Testing
> ---
> 
> `bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.DefaultExecutorWithDockerImageCommandHealthCheck"`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61434: Updated health-checks.md to include description of generalized checks.

[Alexander Rukletsov]

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

(Updated Aug. 15, 2017, 6:17 p.m.)


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


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/health-checks.md d0749347d1f9398004e56b03503d655b5fba75f4 


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

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


Testing
---

https://gist.github.com/rukletsov/517a136cb6a0e8b909db30b36b13eded

Additionally rendered in MacDown.


Thanks,

Alexander Rukletsov

Re: Review Request 60626: Eliminate os::shell calls from HDFS for Windows compatibility.

[Andrew Schwartzmeyer]

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




src/hdfs/hdfs.cpp
Lines 118-120 (original), 118-121 (patched)


This needs to have further error handling. You can't just `get().get()`, as 
it may error.

Also, this subtly changed the style; there's a floating open curly brace.


- Andrew Schwartzmeyer


On July 3, 2017, 12:34 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> ---
> 
> (Updated July 3, 2017, 12:34 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Eliminate os::shell calls from HDFS for Windows compatibility.
> 
> 
> Diffs
> -
> 
>   src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4 
> 
> 
> Diff: https://reviews.apache.org/r/60626/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60625: Normalize file separation characters on Windows when building path.

[Andrew Schwartzmeyer]

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




3rdparty/stout/include/stout/path.hpp
Line 146 (original), 146-152 (patched)


I don't like that this is duplicating logic.

Moreover, this points out to me that perhaps `paths::uri` should have its 
logic split.

`paths::normalize` could handle the `\` -> `/` conversion (also allowing us 
to later handle edge cases), and then could be used here transparently.

Then `paths::uri` could dump `bool addprefix` and just idempotently prepend 
`file://` to a normalized path.


- Andrew Schwartzmeyer


On July 3, 2017, 12:33 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60625/
> ---
> 
> (Updated July 3, 2017, 12:33 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normalize file separation characters on Windows when building path.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 
> 
> 
> Diff: https://reviews.apache.org/r/60625/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60624: Enable HDFS compilation and associated tests.

[Andrew Schwartzmeyer]

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




src/CMakeLists.txt
Line 513 (original), 514 (patched)


Uh.



src/launcher/fetcher.cpp
Lines 267 (patched)


This one is straight-forward. On Windows, there is no "executable 
permissions" so we can simply ignore this logic. With a note of course as to 
why.



src/launcher/fetcher.cpp
Lines 487 (patched)


I'm not suggesting to do this _right now_. But for `chown`, Windows has a 
mappable concept of taking (probably recursive) ownership of the directory as 
the specified user. So we can handle this, we'll just need to implement it.



src/launcher/fetcher.cpp
Lines 564 (patched)


I think we can do the same thing as `os::su` with Windows user 
impersonation. It's a mappable concept.



src/tests/hdfs_tests.cpp
Lines 56 (patched)


Like above, we can safely ignore setting execution permission on Windows. 
My reasoning is that the concept maps to nothing. So doing nothing is 
appropriate I think.


- Andrew Schwartzmeyer


On July 3, 2017, 12:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60624/
> ---
> 
> (Updated July 3, 2017, 12:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that tests are disabled for Windows due to dependence on 'sh'
> shell.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 51b67428f823454665db695ba70a0dc896a7595c 
>   src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
>   src/tests/CMakeLists.txt 9c0acaf43f451dbc9ba5077529a36aa4cef40c34 
>   src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 
> 
> 
> Diff: https://reviews.apache.org/r/60624/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61434: Updated health-checks.md to include description of generalized checks.

[Alexander Rukletsov]

> On Aug. 9, 2017, 5:30 p.m., Avinash sridharan wrote:
> > docs/health-checks.md
> > Lines 204 (patched)
> > 
> >
> > I think we also need to mention that the expectation here is that task 
> > is listening on the `loopback` interface along with any other routeable 
> > interface to which it might be listening.

Good idea.


> On Aug. 9, 2017, 5:30 p.m., Avinash sridharan wrote:
> > docs/health-checks.md
> > Line 265 (original), 494 (patched)
> > 
> >
> > Why do we need to share the `mnt` namespace? This is already done by 
> > the executor for `MesosContainerizer` so why does the `checker` need to do 
> > this if it is running as part of the executor?

Nope, not for the command executor case when `rootfs` is specified.


- Alexander


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


On Aug. 11, 2017, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61434/
> ---
> 
> (Updated Aug. 11, 2017, 12:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7349
> https://issues.apache.org/jira/browse/MESOS-7349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md d0749347d1f9398004e56b03503d655b5fba75f4 
> 
> 
> Diff: https://reviews.apache.org/r/61434/diff/2/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/517a136cb6a0e8b909db30b36b13eded
> 
> Additionally rendered in MacDown.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61573: Made the default executor support signal escalation.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Aug. 14, 2017, 5:19 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61573/
> ---
> 
> (Updated Aug. 14, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This modifies the default executor to perform signal escalation
> via the 'KILL_NESTED_CONTAINER' call i.e., send a SIGTERM followed
> by a SIGKILL after some grace period. Note that support for kill
> policies still needs to be done instead of using a constant grace
> period.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61573/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61668: Added support for kill policies to the default executor.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp
Line 995 (original), 1001 (patched)


We should send a TASK_KILLING update here, and address the TODOs added by 
AlexR.


- Gastón Kleiman


On Aug. 15, 2017, 5:40 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61668/
> ---
> 
> (Updated Aug. 15, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6535
> https://issues.apache.org/jira/browse/MESOS-6535
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61668/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 60623: Convert "file://" URI handling to use new path::uri() function.

[Andrew Schwartzmeyer]

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


Ship it!




Just a note though: I don't see any use in this patch of that `bool addprefix` 
part of the new `paths::uri` interface.

- Andrew Schwartzmeyer


On July 3, 2017, 12:31 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60623/
> ---
> 
> (Updated July 3, 2017, 12:31 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Convert "file://" URI handling to use new path::uri() function.
> 
> 
> Diffs
> -
> 
>   src/tests/credentials_tests.cpp 7eb5abd21a1be35d4739c4928cb922f028cfc9a7 
>   src/tests/fetcher_tests.cpp 99149baa1c7abfabf572a0d0f4512a8e84d1e5be 
>   src/tests/master_contender_detector_tests.cpp 
> f499136c0b981072af5bc8accf2238b7ba4bf430 
>   src/tests/script.cpp 8d40e01da005cb05e7804f0b3975e3e0edb8f3bd 
> 
> 
> Diff: https://reviews.apache.org/r/60623/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61668: Added support for kill policies to the default executor.

[Gastón Kleiman]

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


Fix it, then Ship it!




We could update the test now, to make it take less than 3 seconds to run.


src/launcher/default_executor.cpp
Lines 1005-1006 (original), 1024-1025 (patched)


I think it'd be useful to include the container and task ids.


- Gastón Kleiman


On Aug. 15, 2017, 5:40 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61668/
> ---
> 
> (Updated Aug. 15, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6535
> https://issues.apache.org/jira/browse/MESOS-6535
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61668/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 60622: Add new stout function: path::uri (convert filename to valid URI).

[Andrew Schwartzmeyer]

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




3rdparty/stout/include/stout/path.hpp
Lines 68 (patched)


I don't think I would use `bool addprefix` for this logic. A URI by 
definition is _required_ to have the scheme. In fact, as far as I can tell from 
the RFC, the scheme and the host are the only required parts of a URI:

 scheme:[//[user[:password]@]host[:port]][/path][?query][#fragment]

So using this with `addprefix = false` would _not_ return a URI, and then 
the function no longer makes sense.

What use case drove the additoin of `bool addprefix`, can we solve it in 
another way?



3rdparty/stout/include/stout/path.hpp
Lines 75 (patched)


I don't like duplicating this logic. We should keep the shared logic common 
to one spot, and the platform-specific logic by itself.



3rdparty/stout/include/stout/path.hpp
Lines 77 (patched)


If this is the case, what are we doing on Linux if the path has backslashes 
in it?

I recognize this wasn't already previously handled, but it's a good 
question. Should this function be normalizing as well?


- Andrew Schwartzmeyer


On July 3, 2017, 12:30 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60622/
> ---
> 
> (Updated July 3, 2017, 12:30 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new stout function: path::uri (convert filename to valid URI).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 
>   3rdparty/stout/tests/path_tests.cpp 
> f8c14d5aefe0b49adb778da784143a328c96183d 
> 
> 
> Diff: https://reviews.apache.org/r/60622/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61573: Made the default executor support signal escalation.

[Gastón Kleiman]

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




src/launcher/default_executor.cpp
Line 995 (original), 995 (patched)


We should send a `TASK_KILLING` update here, and address the TODOs added by 
AlexR.


- Gastón Kleiman


On Aug. 14, 2017, 5:19 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61573/
> ---
> 
> (Updated Aug. 14, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This modifies the default executor to perform signal escalation
> via the 'KILL_NESTED_CONTAINER' call i.e., send a SIGTERM followed
> by a SIGKILL after some grace period. Note that support for kill
> policies still needs to be done instead of using a constant grace
> period.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61573/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 61666: Added a test to verify filtering of resource reservations on agent.

[Andrei Budnik]

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

Review request for mesos, Alexander Rojas and Greg Mann.


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


Repository: mesos


Description
---

This patch extends `SlaveAuthorizerTest.FilterStateEndpoint` test to
check that `/state` endpoint shows resource reservations only for
allowed principal.


Diffs
-

  src/tests/slave_authorization_tests.cpp 
536a8efda0cc1ba08285c379b01af6ec64a82117 


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


Testing
---

make check (mac os x, fedora 25)


Thanks,

Andrei Budnik

Re: Review Request 61573: Made the default executor support signal escalation.

[Gastón Kleiman]

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




src/launcher/default_executor.cpp
Line 1054 (original), 1057 (patched)


The task ID is user-generated, so we usually put it in single quotes.



src/launcher/default_executor.cpp
Line 1059 (original), 1064 (patched)


Ditto.


- Gastón Kleiman


On Aug. 14, 2017, 5:19 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61573/
> ---
> 
> (Updated Aug. 14, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This modifies the default executor to perform signal escalation
> via the 'KILL_NESTED_CONTAINER' call i.e., send a SIGTERM followed
> by a SIGKILL after some grace period. Note that support for kill
> policies still needs to be done instead of using a constant grace
> period.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 
> 
> 
> Diff: https://reviews.apache.org/r/61573/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 60620: Modify os::write to write binary files on Windows.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On July 5, 2017, 10:43 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60620/
> ---
> 
> (Updated July 5, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modify os::write to write binary files on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/write.hpp 
> beb5bd83b52565a75e34d32b5bb17951bc799578 
> 
> 
> Diff: https://reviews.apache.org/r/60620/diff/2/
> 
> 
> Testing
> ---
> 
> Built successfully on both Linux (with autotools and cmake) and Windows (with 
> cmake).
> 
> Ran stout-tests and mesos-tests successfully on both Windows and Linux.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61574: Added `kill()` support to the test containerizer interface.

[Anand Mazumdar]

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

(Updated Aug. 15, 2017, 5:41 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Updated review deps


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


Repository: mesos


Description
---

This is needed to make some tests pass that rely on the test
containerizer.


Diffs (updated)
-

  src/tests/containerizer.hpp 4bd40c32625bc1f7998d523c6ee81bf78ac74538 
  src/tests/containerizer.cpp 1d2b6391cf7a7545fa44206c59d05764f3e8cdfb 


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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 61668: Added support for kill policies to the default executor.

[Anand Mazumdar]

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

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


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp c25cc941eefc0cca998a99d76497bfdd05babe92 


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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 61434: Updated health-checks.md to include description of generalized checks.

[Alexander Rukletsov]

> On Aug. 9, 2017, 12:47 a.m., Gastón Kleiman wrote:
> > docs/health-checks.md
> > Lines 266 (patched)
> > 
> >
> > Don't the executors send an empty check status in this case? See 
> > `CommandExecutorCheckTest.CommandCheckTimeout`.
> 
> Alexander Rukletsov wrote:
> Correct. This is an artefact from health-checks-only times and should be 
> updated.

Actually no : ). I meant "no result" as "absence of the result", but I see how 
it is read differently.


- Alexander


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


On Aug. 11, 2017, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61434/
> ---
> 
> (Updated Aug. 11, 2017, 12:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7349
> https://issues.apache.org/jira/browse/MESOS-7349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md d0749347d1f9398004e56b03503d655b5fba75f4 
> 
> 
> Diff: https://reviews.apache.org/r/61434/diff/2/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/517a136cb6a0e8b909db30b36b13eded
> 
> Additionally rendered in MacDown.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61434: Updated health-checks.md to include description of generalized checks.

[Alexander Rukletsov]

> On Aug. 9, 2017, 12:47 a.m., Gastón Kleiman wrote:
> > docs/health-checks.md
> > Lines 266 (patched)
> > 
> >
> > Don't the executors send an empty check status in this case? See 
> > `CommandExecutorCheckTest.CommandCheckTimeout`.

Correct. This is an artefact from health-checks-only times and should be 
updated.


- Alexander


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


On Aug. 11, 2017, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61434/
> ---
> 
> (Updated Aug. 11, 2017, 12:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7349
> https://issues.apache.org/jira/browse/MESOS-7349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md d0749347d1f9398004e56b03503d655b5fba75f4 
> 
> 
> Diff: https://reviews.apache.org/r/61434/diff/2/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/517a136cb6a0e8b909db30b36b13eded
> 
> Additionally rendered in MacDown.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61434: Updated health-checks.md to include description of generalized checks.

[Alexander Rukletsov]

> On Aug. 11, 2017, 6:05 p.m., Greg Mann wrote:
> > docs/health-checks.md
> > Lines 272-278 (patched)
> > 
> >
> > Should we also call out here that setting interval_seconds to zero is a 
> > really bad idea?

Yes, good suggestion.


- Alexander


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


On Aug. 11, 2017, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61434/
> ---
> 
> (Updated Aug. 11, 2017, 12:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7349
> https://issues.apache.org/jira/browse/MESOS-7349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md d0749347d1f9398004e56b03503d655b5fba75f4 
> 
> 
> Diff: https://reviews.apache.org/r/61434/diff/2/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/517a136cb6a0e8b909db30b36b13eded
> 
> Additionally rendered in MacDown.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61434: Updated health-checks.md to include description of generalized checks.

[Alexander Rukletsov]

> On Aug. 11, 2017, 6:05 p.m., Greg Mann wrote:
> > docs/health-checks.md
> > Lines 102 (patched)
> > 
> >
> > s/it/them/

:blush:


- Alexander


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


On Aug. 11, 2017, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61434/
> ---
> 
> (Updated Aug. 11, 2017, 12:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7349
> https://issues.apache.org/jira/browse/MESOS-7349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md d0749347d1f9398004e56b03503d655b5fba75f4 
> 
> 
> Diff: https://reviews.apache.org/r/61434/diff/2/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/517a136cb6a0e8b909db30b36b13eded
> 
> Additionally rendered in MacDown.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61222: Added V1 teardown Call.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Aug. 3, 2017, 5:56 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61222/
> ---
> 
> (Updated Aug. 3, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-6846
> https://issues.apache.org/jira/browse/MESOS-6846
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added V1 call support for teardown operation.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b 
>   include/mesos/v1/master/master.proto 
> c3fb31de2509adcdec8204f8bbe46b46a31540e8 
>   src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 
>   src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 
>   src/master/validation.cpp 279dd5137f767376db29314f116f439ad7ae342f 
> 
> 
> Diff: https://reviews.apache.org/r/61222/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61408: Added test cases for V1 teardown Call.

[Greg Mann]

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




src/tests/api_tests.cpp
Lines 2903-2904 (original), 2903-2905 (patched)


Need one more newline here.



src/tests/api_tests.cpp
Lines 2987 (patched)


s/of 'GET_FRAMEWORKS'/to the 'GET_FRAMEWORKS'/



src/tests/api_tests.cpp
Lines 2992 (patched)


Since you do this at the beginning of the test, you don't need to do it 
here or anywhere below.



src/tests/api_tests.cpp
Lines 3019-3023 (patched)


Not indented far enough, here and below.



src/tests/api_tests.cpp
Lines 3052-3053 (patched)


Could you add a comment here saying that the successful teardown call is 
being submitted? I think it would improve readability a little.


- Greg Mann


On Aug. 15, 2017, 12:07 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61408/
> ---
> 
> (Updated Aug. 15, 2017, 12:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-6846
> https://issues.apache.org/jira/browse/MESOS-6846
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for teardown operation in V1 operator API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
> 
> 
> Diff: https://reviews.apache.org/r/61408/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61664: Libprocess: Added a timeout for send socket operation.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61664]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 15, 2017, 3:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61664/
> ---
> 
> (Updated Aug. 15, 2017, 3:45 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7748
> https://issues.apache.org/jira/browse/MESOS-7748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, a send socket operation can wait forever for
> a send to complete. Clients that drop connections or stop reading
> incoming data, aka "slow reader" attack, can eventually exhaust the
> resources of a libprocess-based application and cause denial of
> service or an OOM event.
> 
> This patch introduces an obligatory timeout for all send socket
> operations, after which the stalled connection is dropped. The
> timeout can be adjusted via the `LIBPROCESS_SOCKET_SEND_TIMEOUT`
> environment variable.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> dcd9c6738816764aae066fe56cd5f468c98fc9bd 
> 
> 
> Diff: https://reviews.apache.org/r/61664/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing with a rogue client.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61262: Added 'heartbeat' event for the operator API.

[Greg Mann]

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



I think this needs a rebase?

- Greg Mann


On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> ---
> 
> (Updated Aug. 14, 2017, 9:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b 
>   include/mesos/v1/master/master.proto 
> c3fb31de2509adcdec8204f8bbe46b46a31540e8 
>   src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 
>   src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 
>   src/tests/api_tests.cpp 75f7a585ec75f1d8b78453dd203bb78822ee7bc9 
> 
> 
> Diff: https://reviews.apache.org/r/61262/diff/4/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Review Request 61664: Libprocess: Added a timeout for send socket operation.

[Alexander Rukletsov]

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

Review request for mesos, Benno Evers, Benjamin Mahler, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Prior to this patch, a send socket operation can wait forever for
a send to complete. Clients that drop connections or stop reading
incoming data, aka "slow reader" attack, can eventually exhaust the
resources of a libprocess-based application and cause denial of
service or an OOM event.

This patch introduces an obligatory timeout for all send socket
operations, after which the stalled connection is dropped. The
timeout can be adjusted via the `LIBPROCESS_SOCKET_SEND_TIMEOUT`
environment variable.


Diffs
-

  3rdparty/libprocess/src/process.cpp dcd9c6738816764aae066fe56cd5f468c98fc9bd 


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


Testing
---

Manual testing with a rogue client.


Thanks,

Alexander Rukletsov

Re: Review Request 61662: Added SlaveRecoveryTest.RebootWithSlaveInfoMismatchAndRestart test.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61661, 61662]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 15, 2017, 3:48 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61662/
> ---
> 
> (Updated Aug. 15, 2017, 3:48 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7795
> https://issues.apache.org/jira/browse/MESOS-7795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new test verifies that the agent removes the 'latest' symlink when it 
> decides to recover as a new one after reboot. And that it can successfully 
> recover again if restarted before checkpointing new agent info.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 
> 
> 
> Diff: https://reviews.apache.org/r/61662/diff/1/
> 
> 
> Testing
> ---
> 
> Ran `make check`. Verified that this test fails without the bugfix patch.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Review Request 61662: Added SlaveRecoveryTest.RebootWithSlaveInfoMismatchAndRestart test.

[Ilya Pronin]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

The new test verifies that the agent removes the 'latest' symlink when it 
decides to recover as a new one after reboot. And that it can successfully 
recover again if restarted before checkpointing new agent info.


Diffs
-

  src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 


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


Testing
---

Ran `make check`. Verified that this test fails without the bugfix patch.


Thanks,

Ilya Pronin

Review Request 61661: Added 'latest' symlink removal.

[Ilya Pronin]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

After a reboot the agent checkpoints new boot ID after recovering and
agent info after registering with the master. If new agent info is
incompatible with the checkpointed one, the agent will recover as a new
one. But if the agent is restarted before registering it will fail to
start again, because new boot ID will already be checkpointed.

This patch fixes it by checkpointing the agent's decision to recover as
a new one by removing the "latest" meta symlink.


Diffs
-

  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

Ran `make check`. A new test is added in the subsequent patch.


Thanks,

Ilya Pronin

Re: Review Request 61654: Added a stream ID to resource provider manager connections.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61275, 61654]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 15, 2017, 12:06 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61654/
> ---
> 
> (Updated Aug. 15, 2017, 12:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to resolve problems with resource providers that erroneously
> try to 'SUBSCRIBE' multiple times a unique stream ID is associated with
> a connected resource provider. Resource providers are expected to send
> this stream ID with every call that isn't a 'SUBSCRIBE' call.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
> 
> 
> Diff: https://reviews.apache.org/r/61654/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61654: Added a stream ID to resource provider manager connections.

[Jan Schlicht]

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

(Updated Aug. 15, 2017, 2:06 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

To be able to resolve problems with resource providers that erroneously
try to 'SUBSCRIBE' multiple times a unique stream ID is associated with
a connected resource provider. Resource providers are expected to send
this stream ID with every call that isn't a 'SUBSCRIBE' call.


Diffs
-

  src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 61272: Added a MockResourceProvider.

[Jan Schlicht]

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

(Updated Aug. 15, 2017, 2:05 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Changed `MockResourceProvider` to provide a `Driver`.


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


Repository: mesos


Description
---

Added a MockResourceProvider.


Diffs (updated)
-

  src/tests/mesos.hpp 6f06261d81870b923b7053daf8205327c4ac6a45 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Review Request 61654: Added a stream ID to resource provider manager connections.

[Jan Schlicht]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

To be able to resolve problems with resource providers that erroneously
try to 'SUBSCRIBE' multiple times a unique stream ID is associated with
a connected resource provider. Resource providers are expected to send
this stream ID with every call that isn't a 'SUBSCRIBE' call.


Diffs
-

  src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 61650: Controlled queued task handling with helpers in the agent.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61639, 61640, 61641, 61642, 61643, 61644, 61645, 61646, 
61647, 61648, 61649, 61638, 61650]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 15, 2017, 7 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61650/
> ---
> 
> (Updated Aug. 15, 2017, 7 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this patch, the enqueueing and dequeueing of tasks in the
> agent are controlled solely by Executor class member functions,
> which simplifies the agent code.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
>   src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
> 
> 
> Diff: https://reviews.apache.org/r/61650/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 61639: Fixed an bug where the agent kills and still launches a task.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-7744 and MESOS-7865
https://issues.apache.org/jira/browse/MESOS-7744
https://issues.apache.org/jira/browse/MESOS-7865


Repository: mesos


Description
---

The following race leads to the agent both killing and launching a task:

  (1) Slave::__run completes, task is now within Executor::queuedTasks.
  (2) Slave::killTask locates the executor based on the task ID residing
  in queuedTasks, calls Slave::statusUpdate() with TASK_KILLED.
  (3) Slave::___run assumes that killed tasks have been removed from
  Executor::queuedTasks, but this now occurs asynchronously in
  Slave::_statusUpdate. So, the agent still sees the queued task
  and delivers it and adds the task to Executor::launchedTasks.
  (3) Slave::_statusUpdate runs, removes the task from
  Executor::launchedTasks and adds it to Executor::terminatedTasks.

The fix applied here is to synchronously transition queued tasks to
a terminal state when statusUpdate is called. This can be done because
for queued tasks, we do not need to retrieve the container status (the
task never reached the container).


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

make check

SlaveTest.KillQueuedTaskDuringExecutorRegistration captures this case, but it 
did not delay retrieving the container status. This test could have been 
updated previously to delay the container status, but now there is no container 
status to delay, so I've left the test alone.


Thanks,

Benjamin Mahler

Review Request 61646: Added a test to verify a fix for MESOS-7863.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

This test ensures that the agent does not drop the TASK_KILLED
update when all pending tasks from a framework are killed.
See MESOS-7863 for the motivating issue.


Diffs
-

  src/tests/slave_tests.cpp 1d9d142ed9e801b79535a2c28f5a94ffbf1bf160 


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


Testing
---

Ran in repetition.


Thanks,

Benjamin Mahler

Review Request 61644: Introduced a CHECK_NOTNONE.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

This is similar to glog's CHECK_NOTNULL.


Diffs
-

  3rdparty/stout/include/stout/check.hpp 
e3cabd413a37be536361b450dc857a725130fde3 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61648: Marked Framework::hasTask in agent as const.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61638: Implemented LinkedHashMap copy construction and assignment.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

The defaults being generated were incorrect. No code is relying
on them today, this was discovered when writing new code.


Diffs
-

  3rdparty/stout/include/stout/linkedhashmap.hpp 
35684e46480761c9b8325525377ca3ca9707990c 
  3rdparty/stout/tests/linkedhashmap_tests.cpp 
267022e3ea601300083a41cc559dfa0adbc9074b 


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


Testing
---

Added a test.


Thanks,

Benjamin Mahler

Review Request 61650: Controlled queued task handling with helpers in the agent.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

With this patch, the enqueueing and dequeueing of tasks in the
agent are controlled solely by Executor class member functions,
which simplifies the agent code.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61640: Improved the reason and message for killed queued tasks updates.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

Previously, if a task was killed before delivery to a registering
executor, we sent REASON_EXECUTOR_UNREGISTERED with a message of
"Unregistered executor". Whereas this case is just that the task
was killed while queued.


Diffs
-

  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
  src/tests/slave_recovery_tests.cpp 9ba6f6005c2edce67e1f63005518ab71ac981423 
  src/tests/slave_tests.cpp 1d9d142ed9e801b79535a2c28f5a94ffbf1bf160 


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


Testing
---

Updated the tests.


Thanks,

Benjamin Mahler

Review Request 61641: Updated Framework::removePendingTask to take only a TaskID.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

This allows the function to be used in the kill task path where we
do not have the TaskInfo or ExecutorInfo available. With this, we
now have all removals going through this function.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61642: Introduced a Framework::idle function in the agent.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

This ensures the call-sites consistently check idleness of the
framework, it also aids readability in that it clarifies that
we remove idle frameworks.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61645: Fixed a bug where TASK_KILLED updates can be dropped by the agent.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-7783 and MESOS-7863
https://issues.apache.org/jira/browse/MESOS-7783
https://issues.apache.org/jira/browse/MESOS-7863


Repository: mesos


Description
---

Per the description of MESOS-7863, there is currently an assumption
that when a pending task is killed, the framework will be stored in
the agent when the launch proceeds for the killed task. When this
assumption does not hold, the TASK_KILLED update will be dropped
due to the frameowrk being unknown when the launch proceeds. This
assumption doesn't hold in two cases:

  (1) Another pending task was killed and we removed the framework
  in 'Slave::run' thinking it was idle, because pending tasks
  was empty (we remove from pending tasks when processing the
  kill). (MESOS-7783 is an example instance of this).

  (2) The last executor terminated without tasks to send terminal
  updates for, or the last terminated executor received its
  last acknowledgement. At this point, we remove the framework
  thinking there were no pending tasks if the task was killed
  (removed from pending).

The fix applied here is to send the status updates from the kill
path rather than the launch path, to be consistent with how we kill
tasks queued within the Executor struct. We ensure that the task
is removed synchronously within the kill path to prevent its launch.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
  src/tests/slave_tests.cpp 1d9d142ed9e801b79535a2c28f5a94ffbf1bf160 


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


Testing
---

Added a test in a subsequent patch.


Thanks,

Benjamin Mahler

Review Request 61643: Renamed Framework::pending to Framework::pendingTasks in agent.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/http.cpp 2d33f0b498c8c819d1aaa6b39ae38b1009fda3e4 
  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61649: Unschedule directories for GC in parallel in the agent.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Repository: mesos


Description
---

Previously, when unscheduling directories for garbage collection,
a dispatch back to the agent was required between each gc result.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
  src/tests/mock_slave.hpp 767ed3d0444258f30b7338981ccc0080166ac5a7 
  src/tests/mock_slave.cpp c435ec76ac44775bf8a87421640649495c16187e 
  src/tests/slave_tests.cpp 1d9d142ed9e801b79535a2c28f5a94ffbf1bf160 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 61647: Added a TODO to correctly represent framework lifecycle in agent.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Currently, we store frameworks as "completed" in the agent when
the agent no longer has any active tasks for executors for the
framework. However, the framework may not be completed according
to the master. Ideally, the master informs the agent when a
framework is _actually_ completed and the agent represents these
"idle" frameworks as being in a non-completed state.


Diffs
-

  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 


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


Testing
---

N/A


Thanks,

Benjamin Mahler