Re: Review Request 63900: Downgraded the logging level of socket shutdown failures.

[Zhitao Li]

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


Ship it!




Ship It!

- Zhitao Li


On Nov. 17, 2017, 6:54 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63900/
> ---
> 
> (Updated Nov. 17, 2017, 6:54 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It often merely shows that a connection is closed before libprocess
> shuts it down so it should be logged at INFO level.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 732aff86bb6f4c56d4eec7107b2cc2aff139c06a 
> 
> 
> Diff: https://reviews.apache.org/r/63900/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63853: Added the "task" prefix to the name of the status update manager files.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63853 was successfully built and tested.

Reviews applied: `['63852', '63853']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63853

- Mesos Reviewbot Windows


On Nov. 16, 2017, 11:49 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63853/
> ---
> 
> (Updated Nov. 16, 2017, 11:49 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the "task" prefix to the name of the status update manager files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/local/local.cpp 99c1b087d75a1759ed61a4e6178e15919bee1e77 
>   src/slave/main.cpp 94431b036524ef4db16a594c80f45b64a45728f7 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
>   src/slave/status_update_manager.hpp 
> 14ead42f22e342d03c117eabe7af635b48052703 
>   src/slave/status_update_manager.cpp 
> e0d029b855de1024882a0db3c2556523240179e5 
>   src/tests/CMakeLists.txt db5e531742fd385a580110736d752be5f879727f 
>   src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
>   src/tests/cluster.cpp d657da6d84e5ea28e0dad180dce069de569c5d38 
>   src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341 
>   src/tests/status_update_manager_tests.cpp 
> 24d6a9951b164369b2a5875e1c54b7e69e22d66b 
> 
> 
> Diff: https://reviews.apache.org/r/63853/diff/4/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Review Request 63900: Downgraded the logging level of socket shutdown failures.

[Jiang Yan Xu]

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

Review request for mesos, Joseph Wu and Zhitao Li.


Repository: mesos


Description
---

It often merely shows that a connection is closed before libprocess
shuts it down so it should be logged at INFO level.


Diffs
-

  3rdparty/libprocess/src/process.cpp 732aff86bb6f4c56d4eec7107b2cc2aff139c06a 


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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 63895: Fixed flaky test UnreachableAgentReregisterAfterFailover test.

[Jiang Yan Xu]

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

Review request for mesos, Alexander Rukletsov and James Peach.


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


Repository: mesos


Description
---

Other tests in this file could probably use the same treatment which I can do 
later.


Diffs
-

  src/tests/slave_tests.cpp a75bb260df223b5b86f31e91eec1b6ba8db00cb2 


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


Testing
---

make check.

I couldn't repro the failure locally but I think the fix makes sense logically 
so it probably is the fix. ;)


Thanks,

Jiang Yan Xu

Re: Review Request 63897: WIP. Add prototype of master changes.

[Mesos Reviewbot Windows]

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



Bad review!

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On Nov. 17, 2017, 8:19 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63897/
> ---
> 
> (Updated Nov. 17, 2017, 8:19 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP. Add prototype of master changes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 859fdff4d9a0604bc506b08af79075084ae23466 
>   include/mesos/v1/mesos.proto cfd4abd3af1d8c9fbd31659161eada9ec9f92282 
>   src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
>   src/master/constants.hpp 725680b1e5f1fb9ca87ec5130d0eff72309432ae 
>   src/master/constants.cpp PRE-CREATION 
>   src/master/detector/standalone.hpp bbe3ce9b05fa3c321988e25a305507e22b938e4c 
>   src/master/detector/zookeeper.hpp 5d0435ee3863e43a999a90595eaa006bbdfdc449 
>   src/master/http.cpp 42139bec519d36316e324ef921157c49cdf2d043 
>   src/master/master.hpp a3c9530e340b14b739da90851227f8eed254b4ac 
>   src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
>   src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 
> 
> 
> Diff: https://reviews.apache.org/r/63897/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63888: Updated composing containerizer tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63888 was successfully built and tested.

Reviews applied: `['63887', '63888']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63888

- Mesos Reviewbot Windows


On Nov. 16, 2017, 8:42 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63888/
> ---
> 
> (Updated Nov. 16, 2017, 8:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7506
> https://issues.apache.org/jira/browse/MESOS-7506
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated composing containerizer tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> b759ba2ec6a975a0acc391cccdcd9a5d6dda72ed 
> 
> 
> Diff: https://reviews.apache.org/r/63888/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60471: Added tests for pruneImages for containerizer and provisioner.

[Gilbert Song]

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




src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 923 (patched)


ditto.



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1199 (patched)


if you just need to prove the image reference is correct. why do you need 
this promise?



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1229 (patched)


const string imageName = "fake-image";



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1231-1235 (patched)


use createDockerImage().



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1237-1242 (patched)


use createContainerInfo()



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1244-1246 (patched)


use createTask()



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1266 (patched)


do you really need to wait for the termination?



src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 1275 (patched)


1u for vector?


- Gilbert Song


On Nov. 16, 2017, 1:36 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60471/
> ---
> 
> (Updated Nov. 16, 2017, 1:36 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-4945
> https://issues.apache.org/jira/browse/MESOS-4945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for pruneImages for containerizer and provisioner.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ce67def65aa65188aff10f5316fcd8b745d0abf2 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 832c81fe88d753b0f00dfab870d7725cf556fcef 
> 
> 
> Diff: https://reviews.apache.org/r/60471/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 56721: Implemented pruneImages with a mark and sweep in docker store.

[Gilbert Song]

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




src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp
Lines 107 (patched)


could you do a grep and fix the excludedImages naming in

metadata_manager.cpp:
provisioner/docker/store.cpp:
mesos_containerizer_tests.cpp:
provisioner_docker_tests.cpp:



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 469 (patched)


you dont need `this` for this lambda



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 461-462 (original), 476-478 (patched)


make them oneline?



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 487 (patched)


this is not safe! you need to defer self.



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 556 (patched)


ditto.



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 599 (patched)


ditto



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 698 (patched)


ditto.



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 737 (patched)


why do you need defer here?



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 739 (patched)


ditto


- Gilbert Song


On Nov. 15, 2017, 8 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56721/
> ---
> 
> (Updated Nov. 15, 2017, 8 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-4945
> https://issues.apache.org/jira/browse/MESOS-4945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes the following changes:
> - add a `pruneImages()` function on the chain of relevant classes;
> - implement prune in docker store;
> - fix mock interface to keep existing tests pass.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> c2689cf36575d1e7c6ef8bd2ab3e25681a2ecfcc 
>   src/slave/containerizer/composing.cpp 
> 64919ef1e61a984956c2280ae6b1890c4d135ad1 
>   src/slave/containerizer/containerizer.hpp 
> 2027bd93cc439ed9b2f544e57183765ee032603b 
>   src/slave/containerizer/docker.hpp 105c0680b6714aded1b3e05699930a072545e681 
>   src/slave/containerizer/docker.cpp 63432a99f58aab00cd8612164e00fe6c3a6cf5dd 
>   src/slave/containerizer/mesos/containerizer.hpp 
> f5d5146de9abdd9804ede6a4b17a965a4b27c05a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> db5f044f7415386c94e4930f365dfc14d403414a 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp 
> 954da1681778878c8aff6150002e52ecb648d1bb 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> d86afd2a6ff0bf87e624db1c99255c85068bf6ab 
>   src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
> 232c027f8f96da0cb30b957bce4607d3695050d2 
>   src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
> cd684b33eb308ce1eeb4539a5b2d51985d835db7 
>   src/slave/containerizer/mesos/provisioner/docker/store.hpp 
> 1cf68665d33bd40a7605d26c96fb7b618407fdd0 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> f357710cb19aec3654b0604f7909d068eaf20095 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 7cba54ce490d1e6e17081cd7e04fd6759ceddb8e 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 01ab83dca79e51b8c96d18ee65705912b0ac8324 
>   src/slave/containerizer/mesos/provisioner/store.cpp 
> cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f 
>   src/tests/containerizer.hpp c98913f1a833aa6a60779b66463610ccd5911bbf 
>   src/tests/containerizer.cpp c6f1ec0b000781270b7c79d5e776575c6df778aa 
>   src/tests/containerizer/mock_containerizer.hpp 
> 5befcccecdb76f3b70993642128745a0134ffa65 
> 
> 
> Diff: https://reviews.apache.org/r/56721/diff/17/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.

[Chun-Hung Hsiao]

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

(Updated Nov. 17, 2017, 1:53 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description (updated)
---

The `ContanierDaemon` class is responsible to monitor if a long-running
service running in a standalone container, and restart the service
container after it exits. It takes the following hook functions:

* `postStartHook`: called after the container is launched every time.
* `postStopHook`: called after the container exits every time.

`ContainerDaemon` does not manage the lifecycle of the contanier it
monitors, so the container persists across `ContainerDaemon`s.


Diffs (updated)
-

  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/slave/container_daemon.hpp PRE-CREATION 
  src/slave/container_daemon.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.

[Chun-Hung Hsiao]

> On Nov. 16, 2017, 10:27 p.m., Jie Yu wrote:
> > src/slave/container_daemon.cpp
> > Lines 166 (patched)
> > 
> >
> > `response.status != http::Status::OK`

Cannot do this. `response.status` is a string.


- Chun-Hung


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


On Nov. 16, 2017, 9:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63680/
> ---
> 
> (Updated Nov. 16, 2017, 9:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8183
> https://issues.apache.org/jira/browse/MESOS-8183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ContainerDaemon` class is responsible to monitor if a long-running
> service running in a standalone container, and restart the service
> container after it exits. It takes the following hook functions:
> 
> * `postStartHook`: called after the container is launched every time.
> * `postStopHook`: called after the container exits every time.
> 
> `ContainerDaemon` does not manage the lifecycle of the container it
> monitors, so the container persists across `ContainerDaemon`s.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/slave/container_daemon.hpp PRE-CREATION 
>   src/slave/container_daemon.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63680/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62997: Added checkpoint and recover capability for layers in provisioner.

[Gilbert Song]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/provisioner.cpp
Line 44 (original), 44 (patched)


kill this line/



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Line 520 (original), 519 (patched)


one more space after `if`


- Gilbert Song


On Nov. 13, 2017, 10:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62997/
> ---
> 
> (Updated Nov. 13, 2017, 10:20 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8079
> https://issues.apache.org/jira/browse/MESOS-8079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checkpoint and recover capability for layers in provisioner.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 689acfcbbb07f071b6195472118a7a7520a44abd 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 466f5edab40732b0d8da4252a71fde9c2956e8e9 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 268dbeb4b18374ef53bc73254bf20ce6830e384f 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 7cba54ce490d1e6e17081cd7e04fd6759ceddb8e 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/62997/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55727: Checkpoint and track docker image layer sizes.

[Zhitao Li]

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

(Updated Nov. 16, 2017, 5:43 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.


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


Repository: mesos


Description
---

This patch added the following capabilities:
1. add the size and fetch timestamp of each docker image layer in metadata;
2. checkpoint and recover size (timestamp will be added later);
3. reuse the `DiskUsageCollector` class to track size of each downloaded store;
4. for upgrade path, reuse same collector to back fill the size of layers
   asynchronously without blocking `recover()`;
5. a counter for number of layers pulled;
6. a gauge for total number of layers;
7. a timer for pull latency distribution in the last hour.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/message.proto 
a55ac9010d5e5b5691ec34b6c342e57811aee4ce 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp 
954da1681778878c8aff6150002e52ecb648d1bb 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
d86afd2a6ff0bf87e624db1c99255c85068bf6ab 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
8058dcb31b9de59fa8d3638b553632235542 


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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in Mesos containerizer.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Nov. 15, 2017, 8:33 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> ---
> 
> (Updated Nov. 15, 2017, 8:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate
>   whether recovered.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> f5d5146de9abdd9804ede6a4b17a965a4b27c05a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> db5f044f7415386c94e4930f365dfc14d403414a 
>   src/slave/containerizer/mesos/paths.hpp 
> 7b67ccf670d991cbc339d74ebf4fccf3ae9a98a8 
>   src/slave/containerizer/mesos/paths.cpp 
> 23f1fee3667aa48e8c31c15e0b69201268e44d37 
> 
> 
> Diff: https://reviews.apache.org/r/55334/diff/16/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 59687: Added tests for recovering ContainerConfig.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Nov. 16, 2017, 1:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59687/
> ---
> 
> (Updated Nov. 16, 2017, 1:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for recovering ContainerConfig.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> d9b1173ad6860ed06e24285551aab9117eddbc96 
> 
> 
> Diff: https://reviews.apache.org/r/59687/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55335: Backfilled required fields in TaskInfo in MesosContainerizer* tests.

[Gilbert Song]

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




src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 103-121 (patched)


why change it back? a mistake?


- Gilbert Song


On Nov. 10, 2017, 11:31 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55335/
> ---
> 
> (Updated Nov. 10, 2017, 11:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests were using a `TaskInfo` field which is missing required
> protobuf fields. After the previous patch begins to checkpoint
> `ContainerConfig`, the incomplete `TaskInfo` cannot be checkpointed,
> thus caused these tests to fail.
> 
> This patch backfills these fields to make these tests pass.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ef424150183c782551a3019afcd4b9d1eb94b863 
> 
> 
> Diff: https://reviews.apache.org/r/55335/diff/8/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="MesosContainerizer*" make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63018: Added filesystem layout for local resource providers.

[Chun-Hung Hsiao]

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

(Updated Nov. 17, 2017, 1:36 a.m.)


Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Jan Schlicht.


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


Repository: mesos


Description
---

A local resource provide can store checkpoints, whose lifecycles should
be tied to the agent, under
`/meta/slaves//resource_providers///
`. Data that persist across agents can be stored under
`/resource_providers//`.


Diffs (updated)
-

  src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
  src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
  src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
  src/resource_provider/storage/provider.hpp 
6de88c2329b358fcf48bc39ddda0132170991c3c 
  src/resource_provider/storage/provider.cpp 
46224997430ac0c568904d80014166a6f059907f 
  src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
  src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 56722: Added a new operator API for `PRUNE_IMAGES`.

[Gilbert Song]

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




src/slave/http.hpp
Lines 306 (patched)


why isn't it a `ContentType acceptType,`?



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


why do you need it?



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


instead of passing in the `acceptType`, we might want to pass the 
containerId and construct the error msg with the containerID.



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


could we add more log? (like `Http::_killContainer()`)



src/slave/http.cpp
Lines 2302-2304 (patched)


one thing to notice is that the current interface returns 
`Future`, so we dont have a way to distringuish a `failure` or `user 
need to drain all containers`. maybe add a TODO?


- Gilbert Song


On Nov. 15, 2017, 8:03 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56722/
> ---
> 
> (Updated Nov. 15, 2017, 8:03 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-4945
> https://issues.apache.org/jira/browse/MESOS-4945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new operator API for `PRUNE_IMAGES`.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 0f92f73ba0f7729f0ba7cd89a692ab3685125e8b 
>   include/mesos/v1/agent/agent.proto 012ffef5f0dd7720fa95ae484c99479aaf256d7b 
>   src/slave/http.hpp a51831cdcebc1998ce6d4c3c19285e598a4ec9a3 
>   src/slave/http.cpp 394e91013dc11e0a79e2e00534864281cc74ad2f 
>   src/slave/validation.cpp 32781fd8f124f71e61744804aec3fe4da59a5df2 
> 
> 
> Diff: https://reviews.apache.org/r/56722/diff/6/
> 
> 
> Testing
> ---
> 
> 1. New unit test to trigger the new call.
> 2. Manually tested that images previous pulled but not running can be purged 
> through new operator API call while active images (running or being pulled) 
> are not affected.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

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

(Updated Nov. 17, 2017, 12:57 a.m.)


Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.


Changes
---

Combined with r62633.


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


Repository: mesos


Description
---

`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.


Diffs (updated)
-

  src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
  src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
  src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
  src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
  src/resource_provider/storage/provider.hpp 
6de88c2329b358fcf48bc39ddda0132170991c3c 
  src/resource_provider/storage/provider.cpp 
46224997430ac0c568904d80014166a6f059907f 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Jie Yu]

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



OK, i think we probably need to do the following given you're reusing the JWT 
authenticator built for executor authn:

1) probably rename `executor_secret_key` to `jwt_secret_key` (using flag alias) 
because this is used 
2) if `jwt_secret_key` is not set, we should not load the JWT authenticator. 
It's already an error if someone set `authencate_http_executor` but not 
`jwt_secret_key`. So this sounds reasonable to me

```
  string httpAuthenticators;
  if (flags.http_authenticators.isSome()) {
httpAuthenticators = flags.http_authenticators.get();
#ifdef USE_SSL_SOCKET
  } else if (flags.jwt_secret_key.isSome()) {
httpAuthenticators =
  string(DEFAULT_BASIC_HTTP_AUTHENTICATOR) + "," +
  string(DEFAULT_JWT_HTTP_AUTHENTICATOR);
#endif // USE_SSL_SOCKET
  } else {
httpAuthenticators = DEFAULT_BASIC_HTTP_AUTHENTICATOR;
  }
```

- Jie Yu


On Nov. 17, 2017, 12:19 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 17, 2017, 12:19 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/6/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 63897: WIP. Add prototype of master changes.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

WIP. Add prototype of master changes.


Diffs
-

  include/mesos/mesos.proto 859fdff4d9a0604bc506b08af79075084ae23466 
  include/mesos/v1/mesos.proto cfd4abd3af1d8c9fbd31659161eada9ec9f92282 
  src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
  src/master/constants.hpp 725680b1e5f1fb9ca87ec5130d0eff72309432ae 
  src/master/constants.cpp PRE-CREATION 
  src/master/detector/standalone.hpp bbe3ce9b05fa3c321988e25a305507e22b938e4c 
  src/master/detector/zookeeper.hpp 5d0435ee3863e43a999a90595eaa006bbdfdc449 
  src/master/http.cpp 42139bec519d36316e324ef921157c49cdf2d043 
  src/master/master.hpp a3c9530e340b14b739da90851227f8eed254b4ac 
  src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
  src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

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

(Updated Nov. 17, 2017, 12:19 a.m.)


Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.


Changes
---

Addressed Joseph's comments.


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


Repository: mesos


Description
---

`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.


Diffs (updated)
-

  src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
  src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

> On Nov. 7, 2017, 2:30 a.m., Joseph Wu wrote:
> > src/resource_provider/daemon.cpp
> > Lines 96-99 (original), 114-118 (patched)
> > 
> >
> > I'd consider an error at this step to be a fatal error (basically a 
> > misconfiguration), so we should fail-fast here and exit the agent.

Fixed in r63376.


> On Nov. 7, 2017, 2:30 a.m., Joseph Wu wrote:
> > src/slave/slave.cpp
> > Line 1234 (original), 1234-1236 (patched)
> > 
> >
> > Consider moving this comment above `start` in `daemon.hpp`.  (With the 
> > side benefit of not needing to duplicate the comment in two places)

Done. This makes sense to me, as the caller would go to the header file to 
check why we need this argument.


- Chun-Hung


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


On Nov. 17, 2017, 12:19 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 17, 2017, 12:19 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/6/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63376: Started `LocalResourceProviderDaemon` after obtaining the slave ID.

[Chun-Hung Hsiao]

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

(Updated Nov. 17, 2017, 12:13 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Constructed the process object during daemon creation but spawned it later when 
`start()` is called. This enables us to accept reload requests even before 
`start()` is called by the agent. Also made the daemon fail-fast if the config 
dir cannot be read.


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


Repository: mesos


Description (updated)
---

The local resource provider daemon is now started aftor we get a slave
id (either through registration or recovery). The slave id will be
eventually passed into each local resource provider for checkpointing
their metadata.


Diffs (updated)
-

  src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
  src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63853: Added the "task" prefix to the name of the status update manager files.

[Gaston Kleiman]

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

(Updated Nov. 16, 2017, 3:49 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Updated ifdef guards.


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


Repository: mesos


Description
---

Added the "task" prefix to the name of the status update manager files.


Diffs (updated)
-

  src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/local/local.cpp 99c1b087d75a1759ed61a4e6178e15919bee1e77 
  src/slave/main.cpp 94431b036524ef4db16a594c80f45b64a45728f7 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/slave/status_update_manager.hpp 14ead42f22e342d03c117eabe7af635b48052703 
  src/slave/status_update_manager.cpp e0d029b855de1024882a0db3c2556523240179e5 
  src/tests/CMakeLists.txt db5e531742fd385a580110736d752be5f879727f 
  src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
  src/tests/cluster.cpp d657da6d84e5ea28e0dad180dce069de569c5d38 
  src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341 
  src/tests/status_update_manager_tests.cpp 
24d6a9951b164369b2a5875e1c54b7e69e22d66b 


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

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


Testing
---

Tests still pass on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 63889: Use annotated tags in vote.sh and release.sh.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On Nov. 16, 2017, 8:46 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63889/
> ---
> 
> (Updated Nov. 16, 2017, 8:46 p.m.)
> 
> 
> Review request for mesos, James Peach and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md d772bcab189f10096806c4ad52768915fb8c8253 
>   support/release.sh 725bee61175e74d9a53aab9e33df1fa8a1a06361 
>   support/vote.sh e9a2347af2d99ed66cabd57845b56323233f50aa 
> 
> 
> Diff: https://reviews.apache.org/r/63889/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 63889: Use annotated tags in vote.sh and release.sh.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63889 was successfully built and tested.

Reviews applied: `['63889']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63889

- Mesos Reviewbot Windows


On Nov. 16, 2017, 8:46 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63889/
> ---
> 
> (Updated Nov. 16, 2017, 8:46 p.m.)
> 
> 
> Review request for mesos, James Peach and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md d772bcab189f10096806c4ad52768915fb8c8253 
>   support/release.sh 725bee61175e74d9a53aab9e33df1fa8a1a06361 
>   support/vote.sh e9a2347af2d99ed66cabd57845b56323233f50aa 
> 
> 
> Diff: https://reviews.apache.org/r/63889/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

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

[Jie Yu]

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


Fix it, then Ship it!





src/hdfs/hdfs.cpp
Line 119 (original), 119 (patched)


i'd kill this line to be consistent with others.



src/hdfs/hdfs.cpp
Lines 134 (patched)


Please put `+` to the end of the line above


- Jie Yu


On Nov. 6, 2017, 6:09 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> ---
> 
> (Updated Nov. 6, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows doesn't support os::shell calls, so reworked the code to use
> libprocess instead.
> 
> 
> Diffs
> -
> 
>   src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4 
> 
> 
> Diff: https://reviews.apache.org/r/60626/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.

[Jie Yu]

> On Nov. 16, 2017, 10:27 p.m., Jie Yu wrote:
> > src/slave/container_daemon.cpp
> > Lines 50-61 (patched)
> > 
> >
> > Hum, so we assume v1 uses token based authentication? What if agent API 
> > uses basic authn?

OK, discussed offline. I'd add some TODO here saying that currently, we assume 
JWT authenticator is used for agent operator API.


- Jie


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


On Nov. 16, 2017, 9:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63680/
> ---
> 
> (Updated Nov. 16, 2017, 9:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8183
> https://issues.apache.org/jira/browse/MESOS-8183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ContainerDaemon` class is responsible to monitor if a long-running
> service running in a standalone container, and restart the service
> container after it exits. It takes the following hook functions:
> 
> * `postStartHook`: called after the container is launched every time.
> * `postStopHook`: called after the container exits every time.
> 
> `ContainerDaemon` does not manage the lifecycle of the container it
> monitors, so the container persists across `ContainerDaemon`s.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/slave/container_daemon.hpp PRE-CREATION 
>   src/slave/container_daemon.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63680/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63852: Added "Task" prefix to status update manager related classes/methods.

[Gaston Kleiman]

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

(Updated Nov. 16, 2017, 3:23 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Updated comments.


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


Repository: mesos


Description
---

We're about to add an offer operation status update manager and a
generic `StatusUpdateManagerProcess` class, which would conflict with
the name of the current classes/methods/variables.


Diffs (updated)
-

  src/local/local.cpp 99c1b087d75a1759ed61a4e6178e15919bee1e77 
  src/messages/messages.proto 33732e28d087a17080eb9aa7d9a91c367172be7f 
  src/slave/main.cpp 94431b036524ef4db16a594c80f45b64a45728f7 
  src/slave/slave.hpp 40442f25ae1b223380e11f8602e6256c9203ef47 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/slave/status_update_manager.hpp 14ead42f22e342d03c117eabe7af635b48052703 
  src/slave/status_update_manager.cpp e0d029b855de1024882a0db3c2556523240179e5 
  src/tests/api_tests.cpp c9c50b91ddd7c3317c1188f878753ad3bd5c3941 
  src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
  src/tests/cluster.cpp d657da6d84e5ea28e0dad180dce069de569c5d38 
  src/tests/master_slave_reconciliation_tests.cpp 
6acf694538c61ce46e513b7b2805da0e0f7758dc 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
  src/tests/mock_slave.hpp 57189cee20511d145ae6b47a4dc2c66a14638dad 
  src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341 
  src/tests/reconciliation_tests.cpp 8c43ffd1cd4ffe1b11d67eb0a1f768c736826d91 
  src/tests/slave_recovery_tests.cpp c864aa92d9ff128a89dbc25653385de25653f56a 
  src/tests/slave_tests.cpp a75bb260df223b5b86f31e91eec1b6ba8db00cb2 
  src/tests/status_update_manager_tests.cpp 
24d6a9951b164369b2a5875e1c54b7e69e22d66b 


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

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


Testing
---

Tests still pass on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 63853: Added the "task" prefix to the name of the status update manager files.

[Gaston Kleiman]

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

(Updated Nov. 16, 2017, 3:23 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Fixed the cmake build.


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


Repository: mesos


Description
---

Added the "task" prefix to the name of the status update manager files.


Diffs (updated)
-

  src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/local/local.cpp 99c1b087d75a1759ed61a4e6178e15919bee1e77 
  src/slave/main.cpp 94431b036524ef4db16a594c80f45b64a45728f7 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/slave/status_update_manager.hpp 14ead42f22e342d03c117eabe7af635b48052703 
  src/slave/status_update_manager.cpp e0d029b855de1024882a0db3c2556523240179e5 
  src/tests/CMakeLists.txt db5e531742fd385a580110736d752be5f879727f 
  src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
  src/tests/cluster.cpp d657da6d84e5ea28e0dad180dce069de569c5d38 
  src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341 
  src/tests/status_update_manager_tests.cpp 
24d6a9951b164369b2a5875e1c54b7e69e22d66b 


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

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


Testing
---

Tests still pass on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 7, 2017, 4:37 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63253/
> ---
> 
> (Updated Nov. 7, 2017, 4:37 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, 
> Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017)
> for proper operation on Windows. Changed to use new URI methods
> introduced as part of MESOS-6705, and used powershell on Windows
> to replace "test" command on Linux.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5078bd4d70698f5cbd14c971fcecfd58f8467a04 
> 
> 
> Diff: https://reviews.apache.org/r/63253/diff/5/
> 
> 
> Testing
> ---
> 
> Ran mesos-tests on both Windows and Linux with no errors.
> 
> Specifically ran mesos-tests with 
> `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the 
> modified tests ran properly.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

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

[Michael Park]

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


Fix it, then Ship it!





src/tests/fetcher_tests.cpp
Line 469 (original), 473 (patched)


These would ideally be a `url::join(...)`, right?
I think for now we just use `strings::join('/', ...)`
in the codebase. Could we do that here and below, for now rather than 
introducing a `url::join`?


- Michael Park


On Nov. 6, 2017, 10:09 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60628/
> ---
> 
> (Updated Nov. 6, 2017, 10:09 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 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).
> See MESOS-8064 for more details.
> 
> In the short term, zip and unzip will work since PowerShell can
> support that natively.
> 
> 
> Diffs
> -
> 
>   src/launcher/CMakeLists.txt c7a83d476efe13d65fa529e7676b6488eb48f44b 
>   src/launcher/fetcher.cpp 5a9e93adbf2e4f0b8ff64a69e62fea5f6a0b8f2d 
>   src/slave/containerizer/fetcher.cpp 
> ba5b0979aa427c5b3dbacf39661d6027da13e0d6 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/fetcher_tests.cpp df9d2d9586a6457004506c4e2a972ccfc912c7c5 
> 
> 
> Diff: https://reviews.apache.org/r/60628/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.

[Chun-Hung Hsiao]

> On Nov. 16, 2017, 10:27 p.m., Jie Yu wrote:
> > src/slave/container_daemon.cpp
> > Lines 157 (patched)
> > 
> >
> > i'd rename this to `launch`, and rename stop to `wait`

Then what should be the name of the public wait function?


- Chun-Hung


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


On Nov. 16, 2017, 9:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63680/
> ---
> 
> (Updated Nov. 16, 2017, 9:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8183
> https://issues.apache.org/jira/browse/MESOS-8183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ContainerDaemon` class is responsible to monitor if a long-running
> service running in a standalone container, and restart the service
> container after it exits. It takes the following hook functions:
> 
> * `postStartHook`: called after the container is launched every time.
> * `postStopHook`: called after the container exits every time.
> 
> `ContainerDaemon` does not manage the lifecycle of the container it
> monitors, so the container persists across `ContainerDaemon`s.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/slave/container_daemon.hpp PRE-CREATION 
>   src/slave/container_daemon.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63680/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.

[Jie Yu]

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




src/slave/container_daemon.cpp
Lines 50-61 (patched)


Hum, so we assume v1 uses token based authentication? What if agent API 
uses basic authn?



src/slave/container_daemon.cpp
Lines 157 (patched)


i'd rename this to `launch`, and rename stop to `wait`



src/slave/container_daemon.cpp
Lines 166 (patched)


`response.status != http::Status::OK`



src/slave/container_daemon.cpp
Lines 195-196 (patched)


Ditto.


- Jie Yu


On Nov. 16, 2017, 9:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63680/
> ---
> 
> (Updated Nov. 16, 2017, 9:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8183
> https://issues.apache.org/jira/browse/MESOS-8183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ContainerDaemon` class is responsible to monitor if a long-running
> service running in a standalone container, and restart the service
> container after it exits. It takes the following hook functions:
> 
> * `postStartHook`: called after the container is launched every time.
> * `postStopHook`: called after the container exits every time.
> 
> `ContainerDaemon` does not manage the lifecycle of the container it
> monitors, so the container persists across `ContainerDaemon`s.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/slave/container_daemon.hpp PRE-CREATION 
>   src/slave/container_daemon.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63680/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63652: Added d_type check in containerizer backend validation.

[Mesos Reviewbot Windows]

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



FAIL: Some Mesos libprocess-tests failed.

Reviews applied: `['63652']`

Failed command: 
`C:\DCOS\mesos\3rdparty\libprocess\src\tests\Debug\libprocess-tests.exe`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63652

Relevant logs:

- 
[libprocess-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63652/logs/libprocess-tests-stdout.log):

```
[   OK ] Scheme/HTTPTest.WWWAuthenticateHeader/0 (1485 ms)
[ RUN  ] Scheme/HTTPTest.WWWAuthenticateHeader/1
[   OK ] Scheme/HTTPTest.WWWAuthenticateHeader/1 (64 ms)
[ RUN  ] Scheme/HTTPTest.Accepts/0
[   OK ] Scheme/HTTPTest.Accepts/0 (775 ms)
[ RUN  ] Scheme/HTTPTest.Accepts/1
[   OK ] Scheme/HTTPTest.Accepts/1 (93 ms)
[--] 34 tests from Scheme/HTTPTest (15158 ms total)

[--] 4 tests from SSLVerifyIPAdd/SSLTest
[ RUN  ] SSLVerifyIPAdd/SSLTest.BasicSameProcess/0
[   OK ] SSLVerifyIPAdd/SSLTest.BasicSameProcess/0 (567 ms)
[ RUN  ] SSLVerifyIPAdd/SSLTest.BasicSameProcess/1
[   OK ] SSLVerifyIPAdd/SSLTest.BasicSameProcess/1 (753 ms)
[ RUN  ] SSLVerifyIPAdd/SSLTest.RequireCertificate/0
[   OK ] SSLVerifyIPAdd/SSLTest.RequireCertificate/0 (1328 ms)
[ RUN  ] SSLVerifyIPAdd/SSLTest.RequireCertificate/1
[   OK ] SSLVerifyIPAdd/SSLTest.RequireCertificate/1 (700 ms)
[--] 4 tests from SSLVerifyIPAdd/SSLTest (3550 ms total)

[--] Global test environment tear-down
[==] 201 tests from 33 test cases ran. (46562 ms total)
[  PASSED  ] 200 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SSLTest.ProtocolMismatch

 1 FAILED TEST
  YOU HAVE 19 DISABLED TESTS

```

- 
[libprocess-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63652/logs/libprocess-tests-stderr.log):

```
I1116 22WARNING: Logging before InitGoogleLogging() is written to STDERR
I1116 22:06:09.056996  3940 openssl.cpp:509] CA directory path unspecified! 
NOTE: Set CA directory path with LIBPROCESS_SSL_CA_DIR=
I1116 22:06:09.061997  3940 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1116 22:06:09.061997  3940 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1116 22:06:09.062996  3940 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\NsgFRg\cert.pem
WARNING: Logging before InitGoogleLogging() is written to STDERR
I1116 22:06:09.818857  4036 openssl.cpp:509] CA directory path unspecified! 
NOTE: Set CA directory path with LIBPROCESS_SSL_CA_DIR=
I1116 22:06:09.819857  4036 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1116 22:06:09.819857  4036 openssl.cpp:526] Will use IP address verification 
in subject alternative name certificate extension.
I1116 22:06:09.819857  4036 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1116 22:06:09.820858  4036 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\fuG4kk\cert.pem
:06:08.748244  2584 openssl.cpp:509] CA directory path unspecified! NOTE: Set 
CA directory path with LIBPROCESS_SSL_CA_DIR=
I1116 22:06:08.765981  2584 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1116 22:06:08.766979  2584 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1116 22:06:08.766979  2584 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\NsgFRg\cert.pem
I1116 22:06:09.520841  2584 openssl.cpp:509] CA directory path unspecified! 
NOTE: Set CA directory path with LIBPROCESS_SSL_CA_DIR=
I1116 22:06:09.520841  2584 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1116 22:06:09.520841  2584 openssl.cpp:526] Will use IP address verification 
in subject alternative name certificate extension.
I1116 22:06:09.520841  2584 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1116 22:06:09.521842  2584 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\fuG4kk\cert.pem
I1116 22:06:10.053632  3636 process.cpp:1052] Failed to accept socket: future 
discarded
```

- Mesos Reviewbot Windows


On Nov. 14, 2017, 9:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 

Re: Review Request 60471: Added tests for pruneImages for containerizer and provisioner.

[Zhitao Li]

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

(Updated Nov. 16, 2017, 9:36 p.m.)


Review request for mesos, Gilbert Song and Jason Lai.


Changes
---

Fix containerierzer->launch return value type.


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


Repository: mesos


Description
---

Added tests for pruneImages for containerizer and provisioner.


Diffs (updated)
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
ce67def65aa65188aff10f5316fcd8b745d0abf2 
  src/tests/containerizer/provisioner_docker_tests.cpp 
832c81fe88d753b0f00dfab870d7725cf556fcef 


Diff: https://reviews.apache.org/r/60471/diff/10/

Changes: https://reviews.apache.org/r/60471/diff/9-10/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 59687: Added tests for recovering ContainerConfig.

[Zhitao Li]

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

(Updated Nov. 16, 2017, 9:34 p.m.)


Review request for mesos, Gilbert Song and Jason Lai.


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


Repository: mesos


Description
---

Added tests for recovering ContainerConfig.


Diffs (updated)
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
d9b1173ad6860ed06e24285551aab9117eddbc96 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 59687: Added tests for recovering ContainerConfig.

[Zhitao Li]

> On Nov. 16, 2017, 7:29 p.m., Gilbert Song wrote:
> > src/tests/containerizer/nested_mesos_containerizer_tests.cpp
> > Lines 1773 (patched)
> > 
> >
> > Did build you test and run it?

Sorry missed this one.


- Zhitao


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


On Nov. 16, 2017, 9:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59687/
> ---
> 
> (Updated Nov. 16, 2017, 9:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for recovering ContainerConfig.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> d9b1173ad6860ed06e24285551aab9117eddbc96 
> 
> 
> Diff: https://reviews.apache.org/r/59687/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.

[Chun-Hung Hsiao]

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

(Updated Nov. 16, 2017, 9:27 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.


Changes
---

Addressed Joseph's comments.


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


Repository: mesos


Description (updated)
---

The `ContainerDaemon` class is responsible to monitor if a long-running
service running in a standalone container, and restart the service
container after it exits. It takes the following hook functions:

* `postStartHook`: called after the container is launched every time.
* `postStopHook`: called after the container exits every time.

`ContainerDaemon` does not manage the lifecycle of the container it
monitors, so the container persists across `ContainerDaemon`s.


Diffs (updated)
-

  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/slave/container_daemon.hpp PRE-CREATION 
  src/slave/container_daemon.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 63862: Ported docker_tests.cpp to Windows.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63862 was successfully built and tested.

Reviews applied: `['63859', '63860', '63861', '63862']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63862

- Mesos Reviewbot Windows


On Nov. 16, 2017, 6:18 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> ---
> 
> (Updated Nov. 16, 2017, 6:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ported the disabled tests to run on Windows. The following tests
> could not be ported due to Windows platform limitations and were
> simply #ifdef'd out:
>   - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
> sandbox).
>   - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
>   - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_tests.cpp 
> 5cabf5a0b0164f9923008677c58806c8931cbc8d 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/1/
> 
> 
> Testing
> ---
> 
> Windows mesos-test:
> [==] 754 tests from 77 test cases ran. (270586 ms total)
> [  PASSED  ] 754 tests.
> 
> 
> Windows DockerTest only:
> [==] 11 tests from 1 test case ran. (85617 ms total)
> [  PASSED  ] 11 tests.
> 
> Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia 
> GPU):
> [==] 12 tests from 1 test case ran. (12413 ms total)
> [  PASSED  ] 12 tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 63889: Use annotated tags in vote.sh and release.sh.

[Kapil Arya]

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

Review request for mesos, James Peach and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/release-guide.md d772bcab189f10096806c4ad52768915fb8c8253 
  support/release.sh 725bee61175e74d9a53aab9e33df1fa8a1a06361 
  support/vote.sh e9a2347af2d99ed66cabd57845b56323233f50aa 


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


Testing
---


Thanks,

Kapil Arya

Review Request 63888: Updated composing containerizer tests.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Updated composing containerizer tests.


Diffs
-

  src/tests/containerizer/composing_containerizer_tests.cpp 
b759ba2ec6a975a0acc391cccdcd9a5d6dda72ed 


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


Testing
---

sudo make check
internal CI


Thanks,

Andrei Budnik

Review Request 63887: Fixed `wait()` and `destroy()` in composing containerizer.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Previously, `wait()` and `destroy()` methods of composing containerizer
returned a future that might be set to `READY` state while the internal
state of composing containerizer is not yet cleaned up.

This patch adds a `termination` promise to `Container` struct,
which is used to return a future from `wait()` and `destroy()` methods.
This promise is set to `READY` state iff related container is
completely destroyed.
`_destroy()` callback is subscribed for a future from `wait()`, which is
called on related containerizer, to propagate a value to the
`termination` promise and do the cleanup.


Diffs
-

  src/slave/containerizer/composing.cpp 
64919ef1e61a984956c2280ae6b1890c4d135ad1 


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


Testing
---

sudo make check
internal CI


Thanks,

Andrei Budnik

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

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

(Updated Nov. 16, 2017, 8:04 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Master will not kill the tasks for non-Partition aware frameworks
when an unreachable agent re-registers with the master.
Master used to send a ShutdownFrameworkMessages to the agent
to kill the tasks from non partition aware frameworks including the
ones that are still registered which was problematic because the offer
from this agent could still go to the same framework which could then
launch new tasks. The agent would then receive tasks of the same
framework and ignore them because it thinks the framework is shutting
down. The framework is not shutting down of course, so from the master
and the scheduler's perspective the task is pending in STAGING forever
until the next agent reregistration, which could happen much later.
This commit fixes the problem by not shutting down the non-partition
aware frameworks on such an agent.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


Diff: https://reviews.apache.org/r/61473/diff/13/

Changes: https://reviews.apache.org/r/61473/diff/12-13/


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

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

(Updated Nov. 16, 2017, 7:43 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Master will not kill the tasks for non-Partition aware frameworks
when an unreachable agent re-registers with the master.
Master used to send a ShutdownFrameworkMessages to the agent
to kill the tasks from non partition aware frameworks including the
ones that are still registered which was problematic because the offer
from this agent could still go to the same framework which could then
launch new tasks. The agent would then receive tasks of the same
framework and ignore them because it thinks the framework is shutting
down. The framework is not shutting down of course, so from the master
and the scheduler’s perspective the task is pending in STAGING forever
until the next agent reregistration, which could happen much later.
This commit fixes the problem by not shutting down the non-partition
aware frameworks on such an agent.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


Diff: https://reviews.apache.org/r/61473/diff/12/

Changes: https://reviews.apache.org/r/61473/diff/11-12/


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

> On Sept. 28, 2017, 9:32 p.m., James Peach wrote:
> > src/master/master.cpp
> > Lines 9476 (patched)
> > 
> >
> > This doesn't seem right to me. Even if the framework is not 
> > partition-aware, the master is still tracking these tasks as unreachable. I 
> > can't see a reason why we would want to fudge the metrics in this case.

We needed to manipulate the metrics for backwards compatibility to continue 
showing only tasks in state  TASK_UNREACHABLE as unreachable since the 
unreachable tasks datastructutre will now hold both tasks in both TASK_LOST and 
TASK_UNREACHABLE.


- Megha


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


On Nov. 16, 2017, 7:20 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Nov. 16, 2017, 7:20 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including the
> ones that are still registered which was problematic because the offer
> from this agent could still go to the same framework which could then
> launch new tasks. The agent would then receive tasks of the same
> framework and ignore them because it thinks the framework is shutting
> down. The framework is not shutting down of course, so from the master
> and the scheduler’s perspective the task is pending in STAGING forever
> until the next agent reregistration, which could happen much later.
> This commit fixes the problem by not shutting down the non-partition
> aware frameworks on such an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
> 
> 
> Diff: https://reviews.apache.org/r/61473/diff/11/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

> On Sept. 29, 2017, 6:19 p.m., Jiang Yan Xu wrote:
> > src/master/master.cpp
> > Lines 7171-7172 (patched)
> > 
> >
> > This could be shortened to one line.
> > 
> > ```
> > update.mutable_status()->set_state(TASK_LOST);
> > ```

I think this one is not applicable any more.


- Megha


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


On Nov. 16, 2017, 7:20 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Nov. 16, 2017, 7:20 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including the
> ones that are still registered which was problematic because the offer
> from this agent could still go to the same framework which could then
> launch new tasks. The agent would then receive tasks of the same
> framework and ignore them because it thinks the framework is shutting
> down. The framework is not shutting down of course, so from the master
> and the scheduler’s perspective the task is pending in STAGING forever
> until the next agent reregistration, which could happen much later.
> This commit fixes the problem by not shutting down the non-partition
> aware frameworks on such an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
> 
> 
> Diff: https://reviews.apache.org/r/61473/diff/11/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 59687: Added tests for recovering ContainerConfig.

[Gilbert Song]

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1773 (patched)


Did build you test and run it?


- Gilbert Song


On Nov. 16, 2017, 9:02 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59687/
> ---
> 
> (Updated Nov. 16, 2017, 9:02 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for recovering ContainerConfig.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> d9b1173ad6860ed06e24285551aab9117eddbc96 
> 
> 
> Diff: https://reviews.apache.org/r/59687/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63862: Ported docker_tests.cpp to Windows.

[Andrew Schwartzmeyer]

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




src/tests/containerizer/docker_tests.cpp
Lines 62 (patched)


If you want to use it, we have a `SLEEP_COMMAND` macro that takes an int 
and generates either `sleep x` on Linuxish and `powershell -noprofile -command 
start-sleep x` on Windows. Using `ping` can be annoying because it emits output.



src/tests/containerizer/docker_tests.cpp
Lines 67 (patched)


You could declare a `Seconds(30)` and not have to use  
`Seconds(DOCKER_WAIT)` below.

It'd be nicer if we didn't have to adjust the timeouts at all...



src/tests/containerizer/docker_tests.cpp
Lines 79-82 (patched)


Is this assumed not fail? I don't know, but if it can fail, we should 
`CHECK_SOME` it before `get()`.



src/tests/containerizer/docker_tests.cpp
Lines 84-85 (patched)


If you don't override it, you get this from the base class MesosTest : 
TemporaryDirectoryTest 
[here](https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L41),
 so you could call `MesosTest::SetUp()` and get the freebies from the base 
classes.



src/tests/containerizer/docker_tests.cpp
Lines 90-91 (patched)


Jesus...



src/tests/containerizer/docker_tests.cpp
Lines 121-124 (patched)


Would it be possible to instead change the default network setting to 
`bridge`? We may have talked about this, I don't remember. What should the 
default be if `host` doesn't work on Windows?



src/tests/containerizer/docker_tests.cpp
Lines 125 (patched)


Nit: indentation (I put clang-format up on #windows).



src/tests/containerizer/docker_tests.cpp
Lines 130-133 (patched)


Nit: indentation.



src/tests/containerizer/docker_tests.cpp
Lines 231 (patched)


We're not checking the status code?



src/tests/containerizer/docker_tests.cpp
Lines 316 (patched)


Ditto.



src/tests/containerizer/docker_tests.cpp
Lines 399-401 (patched)


Would this read better with an `all_of`?



src/tests/containerizer/docker_tests.cpp
Lines 400 (patched)


Should we maybe do `strings::remove(PREFIX, "/", container.name)` for a 
safer test? (Otherwise we're just assuming here that `container.name` will 
always be prefixed with a slash.)



src/tests/containerizer/docker_tests.cpp
Lines 405-412 (patched)


Would this read better with an `any_of`? We're really just checking that at 
least one of the elements of `containers.get()` matches `containerName`.



src/tests/containerizer/docker_tests.cpp
Lines 407 (patched)


Ditto on the "/".



src/tests/containerizer/docker_tests.cpp
Lines 417 (patched)


Oh I see, actually I think you want `find_if` since we're looking for that 
particular `containerName`.



src/tests/containerizer/docker_tests.cpp
Lines 420 (patched)


This is checking that the ID is not an empty string, but the coment says 
we're checking that it hasn't changed. We should probably save the ID earlier 
and check its value matches here.



src/tests/containerizer/docker_tests.cpp
Lines 421 (patched)


Ditto on the "/".



src/tests/containerizer/docker_tests.cpp
Lines 422 (patched)


Sweet.



src/tests/containerizer/docker_tests.cpp
Lines 611 (patched)


Should we get an actual Windows temp directory instead of `C:\tmp`?



src/tests/containerizer/docker_tests.cpp
Lines 578-624 (original), 703-752 (patched)


Wait, why did we remove it from the list of skipped tests?



src/tests/containerizer/docker_tests.cpp
Line 582 (original), 708-709 (patched)


Nit: Join these lines again.



src/tests/containerizer/docker_tests.cpp
Lines 629-630 (original), 757-758 (patched)


Nit: Join the lines 

Re: Review Request 63794: Added a new member field `ipv6` to the `CheckerProcess` class.

[Avinash sridharan]

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


Fix it, then Ship it!




Ship It!


src/checks/checker_process.hpp
Lines 148 (patched)


s/temp/temporary


- Avinash sridharan


On Nov. 16, 2017, 7:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63794/
> ---
> 
> (Updated Nov. 16, 2017, 7:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new member field `ipv6` to the `CheckerProcess` class.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 5d0c36dd3e1edf68527fc109d8c0708f09b7e12f 
>   src/checks/checker_process.cpp 7985e8d4dc6115f81a4daef0ce4eee67f4233c9f 
>   src/checks/health_checker.cpp d4bda6ed1747ae4c970619bbfa336321aeea52ea 
> 
> 
> Diff: https://reviews.apache.org/r/63794/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63652: Added d_type check in containerizer backend validation.

[Meng Zhu]

> On Nov. 14, 2017, 9:32 a.m., James Peach wrote:
> > This is looking pretty good.
> > 
> > You should be able to write a test for this using the 
> > [ROOT_XFS_TestBase](https://github.com/apache/mesos/blob/master/src/tests/containerizer/xfs_quota_tests.cpp)
> >  fixture. Probably a reasonable approach would be to make a XFS filesystem 
> > with and without `d_type` support and verify that `Provisioner::create` 
> > succeeds or fails.
> 
> Meng Zhu wrote:
> Thank you for the quick and informative review, James.
> 
> I was planning to add a unit test. But the problem is that it would 
> require XFS installed. We will have to add a new configuration flag which 
> seems to overkill. Thus I only did manual tests following a similar 
> procedure. 
> 
> What do you think?
> 
> James Peach wrote:
> The XFS isolator tests (see `ROOT_XFS_TestBase`) already deal with all 
> this. They check for XFS being supported on the kernel and have helpers to 
> construct XFS filesystems on demand.

That's right, I thought about this. But this test is only enabled with 
compilation flag `--enable-xfs-disk-isolator` (which checks system XFS 
support). I feel lumping the d_type test under this flag is not appropriate. I 
see three options: (1) wrap the test under this flag. (2) skip the test at the 
moment. (3) introduce a new flag such as `--enable-xfs` which checks system XFS 
support, enables the `d_type` test and servers as a parent flag for 
`--enable-xfs-disk-isolator` (and any future XFS related functions).

What do you think?


- Meng


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


On Nov. 14, 2017, 1:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63652/
> ---
> 
> (Updated Nov. 14, 2017, 1:30 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-8121
> https://issues.apache.org/jira/browse/MESOS-8121
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In unified containerizer, the d_type cannot be 1
> if we are using the overlay fs backend.
> Raise error if user specifies overlayfs as backend.
> Fallback to other backends in the default case and
> raise a warning.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/63652/diff/4/
> 
> 
> Testing
> ---
> 
> Manually tested slave creation with default and overlayfs backends on XFS 
> based work_dir with different ftype mount options.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for agent local resources.

[Jan Schlicht]

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

(Updated Nov. 16, 2017, 7:59 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Fixed a race condition in the parameterized tests (Offers have to be sent out 
after `UpdateSlaveMessage` was handled in the agent).


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


Repository: mesos


Description
---

Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.


Diffs (updated)
-

  src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
  src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/tests/persistent_volume_tests.cpp 
acfeac16884b00581a3523607ff26f44f6dca53a 
  src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

> On Nov. 16, 2017, 6:54 p.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Line 86 (original), 84 (patched)
> > 
> >
> > `AF_UNSPEC` is default. You can just do `net::IP::parse`
> > 
> > Also instead of `parse` can we use `_ip`?

Looks like AlexR already vetoed the use of `_ip`, so I am good with that.


- Avinash


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


On Nov. 16, 2017, 7:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 16, 2017, 7:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

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




src/checks/tcp_connect.cpp
Line 86 (original), 84 (patched)


`AF_UNSPEC` is default. You can just do `net::IP::parse`

Also instead of `parse` can we use `_ip`?



src/checks/tcp_connect.cpp
Lines 90 (patched)


Why do we need this?

`net::IP::in` and `net::IP::in6` already give this functionality we should 
just use those?


- Avinash sridharan


On Nov. 16, 2017, 7:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 16, 2017, 7:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63853: Added the "task" prefix to the name of the status update manager files.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Nov. 15, 2017, 11:42 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63853/
> ---
> 
> (Updated Nov. 15, 2017, 11:42 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the "task" prefix to the name of the status update manager files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/local/local.cpp 99c1b087d75a1759ed61a4e6178e15919bee1e77 
>   src/slave/main.cpp 94431b036524ef4db16a594c80f45b64a45728f7 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
>   src/slave/status_update_manager.hpp 
> 14ead42f22e342d03c117eabe7af635b48052703 
>   src/slave/status_update_manager.cpp 
> e0d029b855de1024882a0db3c2556523240179e5 
>   src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
>   src/tests/cluster.cpp d657da6d84e5ea28e0dad180dce069de569c5d38 
>   src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341 
>   src/tests/status_update_manager_tests.cpp 
> 24d6a9951b164369b2a5875e1c54b7e69e22d66b 
> 
> 
> Diff: https://reviews.apache.org/r/63853/diff/1/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 63852: Added "Task" prefix to status update manager related classes/methods.

[Greg Mann]

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




src/local/local.cpp
Line 102 (original), 102 (patched)


`grep StatusUpdateManager` yields a couple other sites which we might want 
to update:


https://github.com/apache/mesos/blob/c0a47b696291e152c0a4d772b2b38ab2aad8c0bb/src/messages/messages.proto#L72

https://github.com/apache/mesos/blob/c0a47b696291e152c0a4d772b2b38ab2aad8c0bb/src/tests/slave_recovery_tests.cpp#L356

https://github.com/apache/mesos/blob/c0a47b696291e152c0a4d772b2b38ab2aad8c0bb/src/tests/slave_recovery_tests.cpp#L4605

`grep "status update manager"` also yields a number of comments which could 
be updated, if desired; I'll leave that up to you.



src/slave/status_update_manager.cpp
Line 285 (original), 288 (patched)


Update this logging, as elsewhere?



src/slave/status_update_manager.cpp
Line 512 (original), 517 (patched)


You can leave these two parameters on two separate lines, it's more 
consistent with our style elsewhere.



src/tests/cluster.cpp
Line 506 (original), 506 (patched)


s/status/task status/ ?


- Greg Mann


On Nov. 15, 2017, 11:41 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63852/
> ---
> 
> (Updated Nov. 15, 2017, 11:41 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We're about to add an offer operation status update manager and a
> generic `StatusUpdateManagerProcess` class, which would conflict with
> the name of the current classes/methods/variables.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 99c1b087d75a1759ed61a4e6178e15919bee1e77 
>   src/slave/main.cpp 94431b036524ef4db16a594c80f45b64a45728f7 
>   src/slave/slave.hpp 40442f25ae1b223380e11f8602e6256c9203ef47 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
>   src/slave/status_update_manager.hpp 
> 14ead42f22e342d03c117eabe7af635b48052703 
>   src/slave/status_update_manager.cpp 
> e0d029b855de1024882a0db3c2556523240179e5 
>   src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d 
>   src/tests/cluster.cpp d657da6d84e5ea28e0dad180dce069de569c5d38 
>   src/tests/mock_slave.hpp 57189cee20511d145ae6b47a4dc2c66a14638dad 
>   src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341 
>   src/tests/status_update_manager_tests.cpp 
> 24d6a9951b164369b2a5875e1c54b7e69e22d66b 
> 
> 
> Diff: https://reviews.apache.org/r/63852/diff/1/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 63861: Updated networking doc for Windows docker containers.

[Andrew Schwartzmeyer]

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


Fix it, then Ship it!




Nit:

> Windows: Updated Docker networking documentation.

Add add a description (you can see what I mean about the title getting copied 
into the description).


docs/networking.md
Lines 118-123 (patched)


Are the names of the network types on Windows generally capitalized (like 
the Linux ones here are)?


- Andrew Schwartzmeyer


On Nov. 16, 2017, 10:18 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63861/
> ---
> 
> (Updated Nov. 16, 2017, 10:18 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated networking doc for Windows docker containers.
> 
> 
> Diffs
> -
> 
>   docs/networking.md bdd3a762435aae163fc659ccfea7da825983 
> 
> 
> Diff: https://reviews.apache.org/r/63861/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Fixed docker network settings for Windows.

[Andrew Schwartzmeyer]

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


Fix it, then Ship it!




Nit the review title (for consistency):
> Windows: Mapped the Docker network info types.


src/docker/docker.cpp
Lines 742-743 (original), 742-760 (patched)


If there's an MSDN doc we can reference here to explain this mapping, 
that'd be great.



src/docker/docker.cpp
Lines 747-748 (patched)


Nit: 'host' ... 'transparent' for consistency.



src/docker/docker.cpp
Lines 749 (patched)


Nit: "'Host'..." and then no need to escape the double quotes ;)



src/docker/docker.cpp
Lines 756 (patched)


Nit: 'bridge' ... 'nat' for consistency.


- Andrew Schwartzmeyer


On Nov. 16, 2017, 10:18 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Nov. 16, 2017, 10:18 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, if the host network
> type is sent to the Windows agent, it will return an error. If the
> bridge network is sent, then it will be internally converted to nat,
> since they are equivalent.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63859: Fixed signal vals & WEXITSTATUS on Windows stout.

[Andrew Schwartzmeyer]

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



The summary for this could be:

> Windows: Fixed mock signal values.
>
> Also fixed the `WEXITSTATUS` macro to casat the exit code instead of 
> bit-masking it. (Maybe explain why?)


3rdparty/stout/include/stout/windows.hpp
Lines 346-348 (original), 346-348 (patched)


We should add a link to MSDN explaining why these are now set the way they 
are.



3rdparty/stout/include/stout/windows.hpp
Line 377 (original), 378 (patched)


Should this be a static cast?


- Andrew Schwartzmeyer


On Nov. 16, 2017, 10:17 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Nov. 16, 2017, 10:17 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed signal vals & WEXITSTATUS on Windows stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> 7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 
> 
> 
> Diff: https://reviews.apache.org/r/63859/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63652: Added d_type check in containerizer backend validation.

[James Peach]

> On Nov. 14, 2017, 5:32 p.m., James Peach wrote:
> > This is looking pretty good.
> > 
> > You should be able to write a test for this using the 
> > [ROOT_XFS_TestBase](https://github.com/apache/mesos/blob/master/src/tests/containerizer/xfs_quota_tests.cpp)
> >  fixture. Probably a reasonable approach would be to make a XFS filesystem 
> > with and without `d_type` support and verify that `Provisioner::create` 
> > succeeds or fails.
> 
> Meng Zhu wrote:
> Thank you for the quick and informative review, James.
> 
> I was planning to add a unit test. But the problem is that it would 
> require XFS installed. We will have to add a new configuration flag which 
> seems to overkill. Thus I only did manual tests following a similar 
> procedure. 
> 
> What do you think?

The XFS isolator tests (see `ROOT_XFS_TestBase`) already deal with all this. 
They check for XFS being supported on the kernel and have helpers to construct 
XFS filesystems on demand.


- James


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


On Nov. 14, 2017, 9:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63652/
> ---
> 
> (Updated Nov. 14, 2017, 9:30 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-8121
> https://issues.apache.org/jira/browse/MESOS-8121
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In unified containerizer, the d_type cannot be 1
> if we are using the overlay fs backend.
> Raise error if user specifies overlayfs as backend.
> Fallback to other backends in the default case and
> raise a warning.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/63652/diff/4/
> 
> 
> Testing
> ---
> 
> Manually tested slave creation with default and overlayfs backends on XFS 
> based work_dir with different ftype mount options.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 63861: Updated networking doc for Windows docker containers.

[Akash Gupta]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Updated networking doc for Windows docker containers.


Diffs (updated)
-

  docs/networking.md bdd3a762435aae163fc659ccfea7da825983 


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


Testing
---


Thanks,

Akash Gupta

Review Request 63862: Ported docker_tests.cpp to Windows.

[Akash Gupta]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Ported the disabled tests to run on Windows. The following tests
could not be ported due to Windows platform limitations and were
simply #ifdef'd out:
  - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
sandbox).
  - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
  - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).


Diffs (updated)
-

  src/tests/containerizer/docker_tests.cpp 
5cabf5a0b0164f9923008677c58806c8931cbc8d 


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


Testing (updated)
---

Windows mesos-test:
[==] 754 tests from 77 test cases ran. (270586 ms total)
[  PASSED  ] 754 tests.


Windows DockerTest only:
[==] 11 tests from 1 test case ran. (85617 ms total)
[  PASSED  ] 11 tests.

Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia GPU):
[==] 12 tests from 1 test case ran. (12413 ms total)
[  PASSED  ] 12 tests.


Thanks,

Akash Gupta

Review Request 63860: Fixed docker network settings for Windows.

[Akash Gupta]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

The current Network enum in DockerInfo is specific to Linux containers.
Instead of supporting {host, bridge, none, user} networks, Windows
docker supports {nat, none, user} networks. Now, if the host network
type is sent to the Windows agent, it will return an error. If the
bridge network is sent, then it will be internally converted to nat,
since they are equivalent.


Diffs (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


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


Testing (updated)
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Review Request 63859: Fixed signal vals & WEXITSTATUS on Windows stout.

[Akash Gupta]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Fixed signal vals & WEXITSTATUS on Windows stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


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


Testing (updated)
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

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

[Michael Park]

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


Fix it, then Ship it!





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


Let's mark as a `TODO`.



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


Let's mark these as `TODO`s here and below.


- Michael Park


On Nov. 6, 2017, 10:10 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60624/
> ---
> 
> (Updated Nov. 6, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that HDFS tests are disabled for Windows due to dependence on
> 'sh' shell.
> 
> Be aware: HDFS hasn't been tested, although it should work. We will
> formally add support for HDFS at a later date, see MESOS-5460.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 219252f6f82b2d62d024b2ab876fa0ba2f5c8e6c 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 
> 
> 
> Diff: https://reviews.apache.org/r/60624/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60623: Converted "file://" URI handling to use new uri function.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


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

Re: Review Request 60622: Added new stout functions for path and URI conversions.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/uri.hpp
Lines 19 (patched)


This looks like an accidental include?



3rdparty/stout/include/stout/uri.hpp
Lines 31 (patched)


This should be `#ifdef __WINDOWS__`, right?


- Michael Park


On Nov. 6, 2017, 10:08 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60622/
> ---
> 
> (Updated Nov. 6, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new stout function: path::from_uri, which converts to a filepath
> from a URI.
> 
> Also added uri::from_path, which generates a URI from a file path.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/path.hpp 
> 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 
>   3rdparty/stout/include/stout/uri.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/path_tests.cpp 
> f8c14d5aefe0b49adb778da784143a328c96183d 
>   3rdparty/stout/tests/uri_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60622/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 45 (patched)


The backslash at the end check seems a bit odd. Doesn't `isdir` return 
`true` if `/foo/bar/` is a directory? We also wouldn't be catching other cases 
like `/foo/bar/.` here, right?



3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 65 (patched)


I believe this is equivalent to
```
if (!WSUCCEEDED(status))
```



3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 46-47 (patched)


This message doesn't seem to have single-quotes around the paths whereas 
the POSIX version does.


- Michael Park


On Nov. 6, 2017, 10:08 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Nov. 6, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 59687: Added tests for recovering ContainerConfig.

[Zhitao Li]

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

(Updated Nov. 16, 2017, 5:02 p.m.)


Review request for mesos, Gilbert Song and Jason Lai.


Changes
---

Fix type for returned Future.


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


Repository: mesos


Description
---

Added tests for recovering ContainerConfig.


Diffs (updated)
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
d9b1173ad6860ed06e24285551aab9117eddbc96 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 63798: Added resource provider support for all offer operations.

[Jan Schlicht]

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

(Updated Nov. 16, 2017, 3:43 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.


Diffs (updated)
-

  src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/tests/resource_provider_manager_tests.cpp 
ecfe2b4c0952838d6312df603f8eb2f458725175 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 63577: Fixed a task status update race in default executor tests.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Lines 303-305 (patched)


Let's declare those later, together with `testing::Sequence` for task2.



src/tests/default_executor_tests.cpp
Lines 314-317 (original), 330-339 (patched)


Indentation



src/tests/default_executor_tests.cpp
Lines 364-372 (patched)


Indentation again. Could you please carefully check other places in the 
patch?



src/tests/default_executor_tests.cpp
Lines 434-442 (patched)


Indentation.



src/tests/default_executor_tests.cpp
Lines 404-418 (original), 466-471 (patched)


I'd combine these in one section:
```
// Only tasks in the first group should be killed.
AWAIT_READY(killedUpdate1);
AWAIT_READY(killedUpdate2);
ASSERT_TRUE(killedUpdate3.isPending());
```



src/tests/default_executor_tests.cpp
Lines 513-514 (original), 558-560 (patched)


Again, please move these down right before setting `EXPECT_CALL` for task2.



src/tests/default_executor_tests.cpp
Lines 523-526 (original), 585-594 (patched)


Indentation.



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


Indentation.



src/tests/default_executor_tests.cpp
Lines 652-653 (original), 717-719 (patched)


Ditto.



src/tests/default_executor_tests.cpp
Lines 744-753 (patched)


Ditto.



src/tests/default_executor_tests.cpp
Lines 666-698 (original), 796-803 (patched)


The sequence is a bit weird : ). A curious reader might ask themselves why 
you arrange `AWAIT_`s this way. I'd suggest either doing
```
AWAIT_READY(startingUpdate1);
AWAIT_READY(runningUpdate1);
AWAIT_READY(failedUpdate1);

AWAIT_READY(startingUpdate2);
AWAIT_READY(runningUpdate2);
AWAIT_READY(killedUpdate2);
```
or
```
AWAIT_READY(startingUpdate1);
AWAIT_READY(startingUpdate2);

AWAIT_READY(runningUpdate1);
AWAIT_READY(runningUpdate2);

AWAIT_READY(failedUpdate1);
AWAIT_READY(killedUpdate2);
```
Or add a comment explaining why the sequence you suggest is preferable



src/tests/default_executor_tests.cpp
Lines 867-868 (original), 952-953 (patched)


Ditto, move below please



src/tests/default_executor_tests.cpp
Lines 926-928 (original), 1027-1029 (patched)


What happens if a scheduler gets `TASK_FAILED` status update before 
destruction? Will the test fail with an "uninteresting mock function call"? Or 
there will be no failure because the expectation are specific to the state and 
we just get a warning in the log? I believe the latter, but could you please 
check this?



src/tests/default_executor_tests.cpp
Line 1093 (original), 1195-1196 (patched)


Not sure you need this comment now, the code is self describing. If you 
want to keep it, please add the same comment to all cases above : )



src/tests/default_executor_tests.cpp
Lines 1200-1202 (patched)


Ditto, move closer to task2's expectations.



src/tests/mesos.hpp
Lines 2935-2941 (original), 2935-2941 (patched)


This guy does the same as the one below, but for the old driver. Let's 
rename it to `TaskStatusStateEq` for consistency (in a separate patch).

Also, I'm not sure we need this long comment. Let's keep the first sentence 
only.

And s/task/taskInfo



src/tests/mesos.hpp
Lines 2945 (patched)


s/task/taskInfo



src/tests/mesos.hpp
Lines 2952 (patched)


s/state/taskState


- Alexander Rukletsov


On Nov. 15, 2017, 6:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63577/
> ---
> 
> (Updated Nov. 15, 2017, 6:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gaston Kleiman.
> 
> 
> 

Re: Review Request 63796: Made `CheckerProcess` support IPv6 for HTTP/TCP check.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63796 was successfully built and tested.

Reviews applied: `['63794', '63795', '63796']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63796

- Mesos Reviewbot Windows


On Nov. 16, 2017, 7:22 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63796/
> ---
> 
> (Updated Nov. 16, 2017, 7:22 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `CheckerProcess` support IPv6 for HTTP/TCP check.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 7985e8d4dc6115f81a4daef0ce4eee67f4233c9f 
> 
> 
> Diff: https://reviews.apache.org/r/63796/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63831 was successfully built and tested.

Reviews applied: `['63741', '63830', '63831']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63831

- Mesos Reviewbot Windows


On Nov. 16, 2017, 7:05 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 16, 2017, 7:05 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f0fa7754e5968288edb15929efc9d244b177 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>