Review Request 62203: Use a `process::Executor` to ensure safety of asynchronous callbacks.

2017-09-08 Thread Benjamin Hindman 

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

Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.


Repository: mesos


Description
---

When passing callbacks to `scheduler::Mesos` and `executor::Mesos`
from the tests `TestMesos` it's possible that we'll invoke those
callbacks erroneously after we've destructed `TestMesos` because the
callbacks are executed asynchronously. By using a `process::Executor`
we can guarantee that after `TestMesos` is destroyed (and thereby the
`process::Executor` instance is destroyed) we won't execute the
callbacks.


Diffs
-

  src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 62042: Changed `EXPECT` to `ASSERT` when relying on the assertion afterwards.

2017-09-08 Thread Mesos Reviewbot Windows 

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



ERROR: Failed to apply patch 62197. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62042/logs/apply-reviews.log
 for any relevant errors

Reviews applied: [62197, 61982, 62042]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62042/logs

- Mesos Reviewbot Windows


On Sept. 9, 2017, 8:03 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62042/
> ---
> 
> (Updated Sept. 9, 2017, 8:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
> 
> 
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A common pattern in our tests is to check that at least one offer is
> received using:
> 
> 'EXPECT_FALSE(offers->offers().empty())'
> 
> The test then access the first element of the array returned by
> `offers->offers()` to extract information such as the agent ID.
> 
> The execution of a test will not be interrupted when an `EXPECT` fails.
> That means that tests will try to access elements from an empty
> container when an empty offers vector is received.
> 
> `ASSERT` will return from the test method as soon as a check fails.
> 
> This patch makes the tests that follow the aforementioned pattern use
> `ASSERT_FALSE` instead of `EXPECT_FALSE`.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 0902a66772d178505e83b81878b7878a45c7bb25 
>   src/tests/check_tests.cpp cc7be317030d147c5617e784a50e42b0365b9df6 
>   src/tests/container_logger_tests.cpp 
> 97e79792d3ea8023890ad2a705db47f2aeb419cf 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 68b8181b9c6eb9ce2e8c3c8980451364d410516b 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> f7cab58b75994dc38eede6a84556ae73d412e1a1 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> 38fef2d5f677f768a0533d1ac085b1197b3b764d 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> de42be9ae2019c7cdb24a0976a8565631a12fca3 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> a1ff9b5e67fcccb8aa9afd90ca704796c1b0a252 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 76cc007963b4cbe162556d03d3e41a0a5e660167 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> a7028da62b27b109cb455048acbea0976da9a140 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> eec2e05d399d097fcb997bb44e353b9c3e0563f7 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> f7a715812001f61748749c5cecbed9376fc9ef17 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 544eaef1215f309f67ae177249c209afc71c5d5e 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 8bf6a11de85d4dacf081c009ace9e7b78e904849 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 11dc7f596671d373cd0c9b443a1d217053285a63 
>   src/tests/default_executor_tests.cpp 
> 186b8333c02ba3b9257e19437c6d689761085362 
>   src/tests/disk_quota_tests.cpp 3bf0508238a228d86737d6cc899fa68e2046f2e2 
>   src/tests/exception_tests.cpp 0e72cdfce2f12a6961efefb23fe14eadbadce01f 
>   src/tests/fault_tolerance_tests.cpp 
> 5ac38a897fefb6f40d69ca6e27a6f23176e42d36 
>   src/tests/gc_tests.cpp da9a31c30e759e30492963c199da2b16e9f91550 
>   src/tests/health_check_tests.cpp 2c43241a5cc245a1797ea16c68c94638cec6803d 
>   src/tests/hook_tests.cpp c5acbd0fde7cb0923779a925fe2f6a8214be4163 
>   src/tests/http_fault_tolerance_tests.cpp 
> f05aee8200e05b10595fd821413f44bc36839de2 
>   src/tests/master_authorization_tests.cpp 
> a776a5811b66cb93cfc3484c183e815248dea944 
>   src/tests/master_maintenance_tests.cpp 
> 7595e8b1d8360920f61af4685e210c9a9f4569af 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 0b26953c73c3a41e7f5e4e62a41a0756fb2097e2 
>   src/tests/master_tests.cpp 59fbad468ae671a8fd39457632262015989f26c2 
>   src/tests/master_validation_tests.cpp 
> 710b25c1afdff4de7b2eb9b8a38f6856c373fb3c 
>   src/tests/oversubscription_tests.cpp 
> a4c4a6083a2cb55e2d69eba1719555a7a13ee8fb 
>   src/tests/persistent_volume_tests.cpp 
> 3e1d1fe468298186347b07b5882973c5a16231a3 
>   src/tests/reconciliation_tests.cpp 728961874239c7e5ce8accbaeb2a1c86a73a4f0f 
>   src/tests/registrar_zookeeper_tests.cpp 
> 96bae295bdbe839b979b879cc4b11fddcce6ad75 
>   src/tests/reservation_endpoints_tests.cpp 
> 3278732cb130c73be0f20c0204eddaee05123ff9 
>   src/tests/resource_offers_tests.cpp 
> dc9c230548ba49e92228c6625597b2294e65ca30 
>   src/tests/scheduler_driver_tests.cpp 
> d2aff699dc33d2a246b391d6322d271e7f0252b6 
>   src/tests/scheduler_tests.cpp 21f8825c214bb0c331f66bfb9b12a354d6673a43 
>

Re: Review Request 61109: Used the default value when parsing an optional enum field.

2017-09-08 Thread Benjamin Mahler 


> On Sept. 8, 2017, 11:59 p.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert 
> > all invalid enum names into the default value, but AFAICT that is 
> > unavoidable.

Since we're talking about optional enums, it's not obvious to me whether it's 
better to leave it unset or to set it to the default. With a required enum, we 
can't leave it unset so it seems like the default value makes the most sense. 
However, shouldn't the caller specify the behavior they want? Much like 
`JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be 
something like `use_default_for_unknown_enum_values`?


- Benjamin


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


On July 25, 2017, 3:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> ---
> 
> (Updated July 25, 2017, 3:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the default value when parsing an optional enum field.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/61109/diff/1/
> 
> 
> Testing
> ---
> 
> With this patch, when accessing master endpoint with an inexistent enum `xxx` 
> in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 
> 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790   665 http.cpp:1133] HTTP POST for /master/api/v1 from 
> 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006   665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., 
> `UNKNOWN`) will be used.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61982: Cleaned up DefaultExecutor tests.

2017-09-08 Thread Gastón Kleiman 

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

(Updated Sept. 9, 2017, 12:03 a.m.)


Review request for mesos, Anand Mazumdar and Greg Mann.


Changes
---

Used the new `createExecutorInfo` overloads.


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


Repository: mesos


Description
---

Update the DefaultExecutor tests to use test helpers where possible.
Also make the boilerplate initialization code consistent across tests.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp 186b8333c02ba3b9257e19437c6d689761085362 


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

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


Testing
---

`sudo bin/mesos-tests.sh --gtest_filter="*DefaultExecutor*"` succeeds on 
GNU/Linux


Thanks,

Gastón Kleiman

Re: Review Request 62042: Changed `EXPECT` to `ASSERT` when relying on the assertion afterwards.

2017-09-08 Thread Gastón Kleiman 

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

(Updated Sept. 9, 2017, 12:03 a.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.


Changes
---

Improved the patch description.


Summary (updated)
-

Changed `EXPECT` to `ASSERT` when relying on the assertion afterwards.


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


Repository: mesos


Description (updated)
---

A common pattern in our tests is to check that at least one offer is
received using:

'EXPECT_FALSE(offers->offers().empty())'

The test then access the first element of the array returned by
`offers->offers()` to extract information such as the agent ID.

The execution of a test will not be interrupted when an `EXPECT` fails.
That means that tests will try to access elements from an empty
container when an empty offers vector is received.

`ASSERT` will return from the test method as soon as a check fails.

This patch makes the tests that follow the aforementioned pattern use
`ASSERT_FALSE` instead of `EXPECT_FALSE`.


Diffs (updated)
-

  src/tests/api_tests.cpp 0902a66772d178505e83b81878b7878a45c7bb25 
  src/tests/check_tests.cpp cc7be317030d147c5617e784a50e42b0365b9df6 
  src/tests/container_logger_tests.cpp 97e79792d3ea8023890ad2a705db47f2aeb419cf 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
68b8181b9c6eb9ce2e8c3c8980451364d410516b 
  src/tests/containerizer/cpu_isolator_tests.cpp 
f7cab58b75994dc38eede6a84556ae73d412e1a1 
  src/tests/containerizer/environment_secret_isolator_tests.cpp 
38fef2d5f677f768a0533d1ac085b1197b3b764d 
  src/tests/containerizer/io_switchboard_tests.cpp 
de42be9ae2019c7cdb24a0976a8565631a12fca3 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
a1ff9b5e67fcccb8aa9afd90ca704796c1b0a252 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
76cc007963b4cbe162556d03d3e41a0a5e660167 
  src/tests/containerizer/memory_isolator_tests.cpp 
a7028da62b27b109cb455048acbea0976da9a140 
  src/tests/containerizer/memory_pressure_tests.cpp 
eec2e05d399d097fcb997bb44e353b9c3e0563f7 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
f7a715812001f61748749c5cecbed9376fc9ef17 
  src/tests/containerizer/port_mapping_tests.cpp 
544eaef1215f309f67ae177249c209afc71c5d5e 
  src/tests/containerizer/runtime_isolator_tests.cpp 
8bf6a11de85d4dacf081c009ace9e7b78e904849 
  src/tests/containerizer/xfs_quota_tests.cpp 
11dc7f596671d373cd0c9b443a1d217053285a63 
  src/tests/default_executor_tests.cpp 186b8333c02ba3b9257e19437c6d689761085362 
  src/tests/disk_quota_tests.cpp 3bf0508238a228d86737d6cc899fa68e2046f2e2 
  src/tests/exception_tests.cpp 0e72cdfce2f12a6961efefb23fe14eadbadce01f 
  src/tests/fault_tolerance_tests.cpp 5ac38a897fefb6f40d69ca6e27a6f23176e42d36 
  src/tests/gc_tests.cpp da9a31c30e759e30492963c199da2b16e9f91550 
  src/tests/health_check_tests.cpp 2c43241a5cc245a1797ea16c68c94638cec6803d 
  src/tests/hook_tests.cpp c5acbd0fde7cb0923779a925fe2f6a8214be4163 
  src/tests/http_fault_tolerance_tests.cpp 
f05aee8200e05b10595fd821413f44bc36839de2 
  src/tests/master_authorization_tests.cpp 
a776a5811b66cb93cfc3484c183e815248dea944 
  src/tests/master_maintenance_tests.cpp 
7595e8b1d8360920f61af4685e210c9a9f4569af 
  src/tests/master_slave_reconciliation_tests.cpp 
0b26953c73c3a41e7f5e4e62a41a0756fb2097e2 
  src/tests/master_tests.cpp 59fbad468ae671a8fd39457632262015989f26c2 
  src/tests/master_validation_tests.cpp 
710b25c1afdff4de7b2eb9b8a38f6856c373fb3c 
  src/tests/oversubscription_tests.cpp a4c4a6083a2cb55e2d69eba1719555a7a13ee8fb 
  src/tests/persistent_volume_tests.cpp 
3e1d1fe468298186347b07b5882973c5a16231a3 
  src/tests/reconciliation_tests.cpp 728961874239c7e5ce8accbaeb2a1c86a73a4f0f 
  src/tests/registrar_zookeeper_tests.cpp 
96bae295bdbe839b979b879cc4b11fddcce6ad75 
  src/tests/reservation_endpoints_tests.cpp 
3278732cb130c73be0f20c0204eddaee05123ff9 
  src/tests/resource_offers_tests.cpp dc9c230548ba49e92228c6625597b2294e65ca30 
  src/tests/scheduler_driver_tests.cpp d2aff699dc33d2a246b391d6322d271e7f0252b6 
  src/tests/scheduler_tests.cpp 21f8825c214bb0c331f66bfb9b12a354d6673a43 
  src/tests/slave_authorization_tests.cpp 
30eceae0920351cffc9c3393c6d08917c4041c1a 
  src/tests/slave_recovery_tests.cpp 0e46748be266809c413fb10fc447516c51504fce 
  src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 
  src/tests/status_update_manager_tests.cpp 
6922ee353123bc024c66a6bc4fadc96450287447 
  src/tests/teardown_tests.cpp 8834333bbcdb6a04a95a6ed9632ad4ead0791f76 


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

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


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gastón Kleiman

Re: Review Request 62197: Added new overloads for the `createExecutorInfo` test helper method.

2017-09-08 Thread Gastón Kleiman 

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

(Updated Sept. 9, 2017, 12:03 a.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.


Changes
---

Added more overloads.


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


Repository: mesos


Description (updated)
---

These new overloads make it possible to specify a framework ID, executor
resources as a proto, and the executor ID also as a proto.


Diffs (updated)
-

  src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 


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

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


Testing
---

`make check` still passes on GNU/Linux.


Thanks,

Gastón Kleiman

Re: Review Request 61174: Added a test `ProtobufTest.ParseJSONOptionalEnum`.

2017-09-08 Thread James Peach 

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


Ship it!




Ship It!

- James Peach


On July 27, 2017, 8:19 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61174/
> ---
> 
> (Updated July 27, 2017, 8:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ProtobufTest.ParseJSONOptionalEnum`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> 8877e8934e0f7875bfedcfa88b491ce4b13ca44f 
>   3rdparty/stout/tests/protobuf_tests.proto 
> d16726aa8060aea2b830040b20dbdd467c801483 
> 
> 
> Diff: https://reviews.apache.org/r/61174/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61109: Used the default value when parsing an optional enum field.

2017-09-08 Thread James Peach 

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


Ship it!




This looks pretty reasonable to me. It's unfortunate that this will convert all 
invalid enum names into the default value, but AFAICT that is unavoidable.

- James Peach


On July 25, 2017, 3:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> ---
> 
> (Updated July 25, 2017, 3:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the default value when parsing an optional enum field.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/61109/diff/1/
> 
> 
> Testing
> ---
> 
> With this patch, when accessing master endpoint with an inexistent enum `xxx` 
> in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 
> 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790   665 http.cpp:1133] HTTP POST for /master/api/v1 from 
> 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006   665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., 
> `UNKNOWN`) will be used.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 62197: Added new overloads for the `createExecutorInfo` test helper method.

2017-09-08 Thread Mesos Reviewbot Windows 

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



ERROR: Failed to apply patch 62197. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62197/logs/apply-reviews.log
 for any relevant errors

Reviews applied: [62197]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62197/logs

- Mesos Reviewbot Windows


On Sept. 8, 2017, 10:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62197/
> ---
> 
> (Updated Sept. 8, 2017, 10:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
> 
> 
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These new overloads make it possible to specify a framework ID and to
> pass resources as `Resources` instead of as a string.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 
> 
> 
> Diff: https://reviews.apache.org/r/62197/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` still passes on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 62176: Added cmake dependency check for libsasl2 on non-Windows platforms.

2017-09-08 Thread Mesos Reviewbot Windows 

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



FAIL: Mesos tests failed to build. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs/mesos-tests-cmake-build.log
 for any relevant errors

Reviews applied: [62105, 62106, 62176]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs

- Mesos Reviewbot Windows


On Sept. 8, 2017, 10:35 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> ---
> 
> (Updated Sept. 8, 2017, 10:35 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.
> 
> 
> Bugs: MESOS-3110
> https://issues.apache.org/jira/browse/MESOS-3110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cmake dependency check for libsasl2 on non-Windows platforms.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7 
> 
> 
> Diff: https://reviews.apache.org/r/62176/diff/2/
> 
> 
> Testing
> ---
> 
> I tested this on my Ubuntu 16.04 system. When the libsasl2 library doesn't 
> exist, it fails the cmake configure/build.  I did not test this on any other 
> platform, but this code is in a "if (NOT WIN32)" block and won't affect a 
> Windows build.  I'm uncertain if there is much support for other kinds of 
> builds (like Mac OS), but this should be platform independent.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 62003: Added `network/ports` isolator nested container tests.

2017-09-08 Thread Mesos Reviewbot Windows 

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



ERROR: Failed to apply patch 60491. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003/logs/apply-reviews.log
 for any relevant errors

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

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003/logs

- Mesos Reviewbot Windows


On Sept. 8, 2017, 10:44 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> ---
> 
> (Updated Sept. 8, 2017, 10:44 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 
> 
> 
> Diff: https://reviews.apache.org/r/62003/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 62197: Added new overloads for the `createExecutorInfo` test helper method.

2017-09-08 Thread Gastón Kleiman 

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

Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.


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


Repository: mesos


Description
---

These new overloads make it possible to specify a framework ID and to
pass resources as `Resources` instead of as a string.


Diffs
-

  src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 


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


Testing
---

`make check` still passes on GNU/Linux.


Thanks,

Gastón Kleiman

Re: Review Request 62196: Updated the HTTP executor authentication docs.

2017-09-08 Thread James Peach 

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


Ship it!




Ship It!

- James Peach


On Sept. 8, 2017, 10:48 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62196/
> ---
> 
> (Updated Sept. 8, 2017, 10:48 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the HTTP executor authentication docs.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md a96de6ecb4cdbd6010236c00acf38adef538176d 
> 
> 
> Diff: https://reviews.apache.org/r/62196/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 62196: Updated the HTTP executor authentication docs.

2017-09-08 Thread Greg Mann 

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

Review request for mesos and James Peach.


Repository: mesos


Description
---

Updated the HTTP executor authentication docs.


Diffs
-

  docs/authentication.md a96de6ecb4cdbd6010236c00acf38adef538176d 


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


Testing
---


Thanks,

Greg Mann

Re: Review Request 62003: Added `network/ports` isolator nested container tests.

2017-09-08 Thread James Peach 

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

(Updated Sept. 8, 2017, 10:44 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added `network/ports` isolator nested container tests using the v1
TaskGroups API. This tests that rogue port usage by a nested task is
detected both with and without agent recovery, and that a well-behaved
task is preserved across agent recovery.


Diffs (updated)
-

  src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 
  src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 


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

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


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 61536: Added network ports isolator socket utilities tests.

2017-09-08 Thread James Peach 

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

(Updated Sept. 8, 2017, 10:42 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added some basic tests for the network ports isolator socket
utilities. We test that we can enumerate our own sockets and
use that to figure out what ports we are listening on.


Diffs (updated)
-

  src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900 
  src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 


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

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


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 62176: Added cmake dependency check for libsasl2 on non-Windows platforms.

2017-09-08 Thread John Kordich via Review Board 

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

(Updated Sept. 8, 2017, 10:35 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.


Changes
---

Addressed suggested comments


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


Repository: mesos


Description
---

Added cmake dependency check for libsasl2 on non-Windows platforms.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7 


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

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


Testing
---

I tested this on my Ubuntu 16.04 system. When the libsasl2 library doesn't 
exist, it fails the cmake configure/build.  I did not test this on any other 
platform, but this code is in a "if (NOT WIN32)" block and won't affect a 
Windows build.  I'm uncertain if there is much support for other kinds of 
builds (like Mac OS), but this should be platform independent.


Thanks,

John Kordich

Re: Review Request 62106: Enabled CRAM MD5 Authentication on Windows and associated tests.

2017-09-08 Thread John Kordich via Review Board 

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

(Updated Sept. 8, 2017, 10:13 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.


Changes
---

Addressed new comments


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


Repository: mesos


Description
---

Enabled CRAM MD5 Authentication on Windows and associated tests.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 4cb8dddf93694722af2377c4a55a195260c8a0cb 
  docs/configuration-cmake.md d2eb571c9458c7fade415e8077c676dd34e63242 
  docs/windows.md b47ba5a0db3d41d6f4bb002283b40622a2bb3e0f 
  src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
  src/Makefile.am a4f2ee008eba263fe52c4e58c305e3fb9ba42f6b 
  src/authentication/cram_md5/authenticatee.cpp 
7b3f767e29163de489018081089e67a6f22971c5 
  src/authentication/cram_md5/authenticator.cpp 
2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c 
  src/authentication/cram_md5/auxprop.hpp 
dedcbe514283dd86a924d92ccdd17173ebe878cb 
  src/master/master.cpp 14d94cfa0e96881f77dd6e0c3449fdc0613e36ce 
  src/sched/sched.cpp ef73c1dccfd736b79f40a057951f022df7f60644 
  src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd 
  src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
  src/tests/authentication_tests.cpp b04be76d23788fbde337eaf85716ec7436ade691 
  src/tests/mesos.cpp fc7f8cb09e223000ea160aa4ab83a003cf4af2fb 
  support/windows-build.bat 100013ed68ecc2f431c20a3f081f8aa6eca961ef 


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

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


Testing
---

Ran the automake build and the cmake build on Linux, as well as the cmake build 
on Windows, with no errors.  Ran tests on both platforms as well.  Note: The 
mesos-3rdparty pull request which contains the cyrus-sasl package needs to be 
merged to master first before these patches will work. (Or, have the cyrus-sasl 
tarball available in a local 3rdparty directory, configured with mesos)


Thanks,

John Kordich

Re: Review Request 62105: Enabled building of the Cyrus SASL CRAM MD5 lib on Windows.

2017-09-08 Thread John Kordich via Review Board 

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

(Updated Sept. 8, 2017, 10:13 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.


Changes
---

Addressed new comments


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


Repository: mesos


Description
---

Enabled building of the Cyrus SASL CRAM MD5 lib on Windows.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7 
  3rdparty/cmake/Versions.cmake 0b62ae96817ade616f8dfc52cf8df995f8fd8927 
  3rdparty/cyrus-sasl-2.1.27rc3.patch PRE-CREATION 


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

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


Testing
---


Thanks,

John Kordich

Re: Review Request 62042: Change `EXPECT` to `ASSERT` when relying on the assertion afterwards.

2017-09-08 Thread Greg Mann 

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



Could you just update the description with some info about the specific case 
you're fixing here?

- Greg Mann


On Sept. 7, 2017, 6:09 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62042/
> ---
> 
> (Updated Sept. 7, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
> 
> 
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Change `EXPECT` to `ASSERT` when relying on the assertion afterwards.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 0902a66772d178505e83b81878b7878a45c7bb25 
>   src/tests/check_tests.cpp cc7be317030d147c5617e784a50e42b0365b9df6 
>   src/tests/container_logger_tests.cpp 
> 97e79792d3ea8023890ad2a705db47f2aeb419cf 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 68b8181b9c6eb9ce2e8c3c8980451364d410516b 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> f7cab58b75994dc38eede6a84556ae73d412e1a1 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> 38fef2d5f677f768a0533d1ac085b1197b3b764d 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> de42be9ae2019c7cdb24a0976a8565631a12fca3 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> a1ff9b5e67fcccb8aa9afd90ca704796c1b0a252 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 1ad9464e99b7dd4dfd2c9bd1a4c6c9c5cce615bd 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> a7028da62b27b109cb455048acbea0976da9a140 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> eec2e05d399d097fcb997bb44e353b9c3e0563f7 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> f7a715812001f61748749c5cecbed9376fc9ef17 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 544eaef1215f309f67ae177249c209afc71c5d5e 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 8bf6a11de85d4dacf081c009ace9e7b78e904849 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 11dc7f596671d373cd0c9b443a1d217053285a63 
>   src/tests/default_executor_tests.cpp 
> 186b8333c02ba3b9257e19437c6d689761085362 
>   src/tests/disk_quota_tests.cpp 3bf0508238a228d86737d6cc899fa68e2046f2e2 
>   src/tests/exception_tests.cpp 0e72cdfce2f12a6961efefb23fe14eadbadce01f 
>   src/tests/fault_tolerance_tests.cpp 
> 5ac38a897fefb6f40d69ca6e27a6f23176e42d36 
>   src/tests/gc_tests.cpp da9a31c30e759e30492963c199da2b16e9f91550 
>   src/tests/health_check_tests.cpp 2c43241a5cc245a1797ea16c68c94638cec6803d 
>   src/tests/hook_tests.cpp c5acbd0fde7cb0923779a925fe2f6a8214be4163 
>   src/tests/http_fault_tolerance_tests.cpp 
> f05aee8200e05b10595fd821413f44bc36839de2 
>   src/tests/master_authorization_tests.cpp 
> a776a5811b66cb93cfc3484c183e815248dea944 
>   src/tests/master_maintenance_tests.cpp 
> 7595e8b1d8360920f61af4685e210c9a9f4569af 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 0b26953c73c3a41e7f5e4e62a41a0756fb2097e2 
>   src/tests/master_tests.cpp 59fbad468ae671a8fd39457632262015989f26c2 
>   src/tests/master_validation_tests.cpp 
> 710b25c1afdff4de7b2eb9b8a38f6856c373fb3c 
>   src/tests/oversubscription_tests.cpp 
> a4c4a6083a2cb55e2d69eba1719555a7a13ee8fb 
>   src/tests/persistent_volume_tests.cpp 
> 3e1d1fe468298186347b07b5882973c5a16231a3 
>   src/tests/reconciliation_tests.cpp 728961874239c7e5ce8accbaeb2a1c86a73a4f0f 
>   src/tests/registrar_zookeeper_tests.cpp 
> 96bae295bdbe839b979b879cc4b11fddcce6ad75 
>   src/tests/reservation_endpoints_tests.cpp 
> 3278732cb130c73be0f20c0204eddaee05123ff9 
>   src/tests/resource_offers_tests.cpp 
> dc9c230548ba49e92228c6625597b2294e65ca30 
>   src/tests/scheduler_driver_tests.cpp 
> d2aff699dc33d2a246b391d6322d271e7f0252b6 
>   src/tests/scheduler_tests.cpp 21f8825c214bb0c331f66bfb9b12a354d6673a43 
>   src/tests/slave_authorization_tests.cpp 
> 30eceae0920351cffc9c3393c6d08917c4041c1a 
>   src/tests/slave_recovery_tests.cpp 0e46748be266809c413fb10fc447516c51504fce 
>   src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 
>   src/tests/status_update_manager_tests.cpp 
> 6922ee353123bc024c66a6bc4fadc96450287447 
>   src/tests/teardown_tests.cpp 8834333bbcdb6a04a95a6ed9632ad4ead0791f76 
> 
> 
> Diff: https://reviews.apache.org/r/62042/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 62042: Change `EXPECT` to `ASSERT` when relying on the assertion afterwards.

2017-09-08 Thread Greg Mann 

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


Ship it!




Ship It!

- Greg Mann


On Sept. 7, 2017, 6:09 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62042/
> ---
> 
> (Updated Sept. 7, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
> 
> 
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Change `EXPECT` to `ASSERT` when relying on the assertion afterwards.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 0902a66772d178505e83b81878b7878a45c7bb25 
>   src/tests/check_tests.cpp cc7be317030d147c5617e784a50e42b0365b9df6 
>   src/tests/container_logger_tests.cpp 
> 97e79792d3ea8023890ad2a705db47f2aeb419cf 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 68b8181b9c6eb9ce2e8c3c8980451364d410516b 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> f7cab58b75994dc38eede6a84556ae73d412e1a1 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> 38fef2d5f677f768a0533d1ac085b1197b3b764d 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> de42be9ae2019c7cdb24a0976a8565631a12fca3 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> a1ff9b5e67fcccb8aa9afd90ca704796c1b0a252 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 1ad9464e99b7dd4dfd2c9bd1a4c6c9c5cce615bd 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> a7028da62b27b109cb455048acbea0976da9a140 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> eec2e05d399d097fcb997bb44e353b9c3e0563f7 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> f7a715812001f61748749c5cecbed9376fc9ef17 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 544eaef1215f309f67ae177249c209afc71c5d5e 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 8bf6a11de85d4dacf081c009ace9e7b78e904849 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 11dc7f596671d373cd0c9b443a1d217053285a63 
>   src/tests/default_executor_tests.cpp 
> 186b8333c02ba3b9257e19437c6d689761085362 
>   src/tests/disk_quota_tests.cpp 3bf0508238a228d86737d6cc899fa68e2046f2e2 
>   src/tests/exception_tests.cpp 0e72cdfce2f12a6961efefb23fe14eadbadce01f 
>   src/tests/fault_tolerance_tests.cpp 
> 5ac38a897fefb6f40d69ca6e27a6f23176e42d36 
>   src/tests/gc_tests.cpp da9a31c30e759e30492963c199da2b16e9f91550 
>   src/tests/health_check_tests.cpp 2c43241a5cc245a1797ea16c68c94638cec6803d 
>   src/tests/hook_tests.cpp c5acbd0fde7cb0923779a925fe2f6a8214be4163 
>   src/tests/http_fault_tolerance_tests.cpp 
> f05aee8200e05b10595fd821413f44bc36839de2 
>   src/tests/master_authorization_tests.cpp 
> a776a5811b66cb93cfc3484c183e815248dea944 
>   src/tests/master_maintenance_tests.cpp 
> 7595e8b1d8360920f61af4685e210c9a9f4569af 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 0b26953c73c3a41e7f5e4e62a41a0756fb2097e2 
>   src/tests/master_tests.cpp 59fbad468ae671a8fd39457632262015989f26c2 
>   src/tests/master_validation_tests.cpp 
> 710b25c1afdff4de7b2eb9b8a38f6856c373fb3c 
>   src/tests/oversubscription_tests.cpp 
> a4c4a6083a2cb55e2d69eba1719555a7a13ee8fb 
>   src/tests/persistent_volume_tests.cpp 
> 3e1d1fe468298186347b07b5882973c5a16231a3 
>   src/tests/reconciliation_tests.cpp 728961874239c7e5ce8accbaeb2a1c86a73a4f0f 
>   src/tests/registrar_zookeeper_tests.cpp 
> 96bae295bdbe839b979b879cc4b11fddcce6ad75 
>   src/tests/reservation_endpoints_tests.cpp 
> 3278732cb130c73be0f20c0204eddaee05123ff9 
>   src/tests/resource_offers_tests.cpp 
> dc9c230548ba49e92228c6625597b2294e65ca30 
>   src/tests/scheduler_driver_tests.cpp 
> d2aff699dc33d2a246b391d6322d271e7f0252b6 
>   src/tests/scheduler_tests.cpp 21f8825c214bb0c331f66bfb9b12a354d6673a43 
>   src/tests/slave_authorization_tests.cpp 
> 30eceae0920351cffc9c3393c6d08917c4041c1a 
>   src/tests/slave_recovery_tests.cpp 0e46748be266809c413fb10fc447516c51504fce 
>   src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 
>   src/tests/status_update_manager_tests.cpp 
> 6922ee353123bc024c66a6bc4fadc96450287447 
>   src/tests/teardown_tests.cpp 8834333bbcdb6a04a95a6ed9632ad4ead0791f76 
> 
> 
> Diff: https://reviews.apache.org/r/62042/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61982: Cleaned up DefaultExecutor tests.

2017-09-08 Thread Greg Mann 

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




src/tests/default_executor_tests.cpp
Line 178 (original), 166 (patched)


Hmmm similar to my comment below, I wonder if a new overload could also 
accept the ExecutorID directly. This might require another template parameter 
for `createExecutorInfo`? Not sure if it's worth it.



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


H I wonder if we should add another overload for `createExecutorInfo` 
so that we don't need to stringify the resources here?



src/tests/default_executor_tests.cpp
Line 180 (original), 171 (patched)


Would it also make sense to add a FrameworkID param to the helper to avoid 
this line?


- Greg Mann


On Aug. 30, 2017, 1:05 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61982/
> ---
> 
> (Updated Aug. 30, 2017, 1:05 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update the DefaultExecutor tests to use test helpers where possible.
> Also make the boilerplate initialization code consistent across tests.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 186b8333c02ba3b9257e19437c6d689761085362 
> 
> 
> Diff: https://reviews.apache.org/r/61982/diff/3/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*DefaultExecutor*"` succeeds on 
> GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 62187: This patch is needed to triger Apache CI build.

2017-09-08 Thread Mesos Reviewbot Windows 

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



FAIL: Some Mesos tests failed. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62187/logs/mesos-tests-stdout.log
 and 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62187/logs/mesos-tests-stderr.log
 for any relevant errors

Reviews applied: [62187]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62187/logs

- Mesos Reviewbot Windows


On Sept. 8, 2017, 8:52 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62187/
> ---
> 
> (Updated Sept. 8, 2017, 8:52 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I need to check potential security issue caused by creating unix
> sockets in /tmp directory, which leads to failed tests:
> [ FAILED ] CommandExecutorCheckTest.CommandCheckDeliveredAndReconciled
> [ FAILED ] CommandExecutorCheckTest.CommandCheckStatusChange
> [ FAILED ] DefaultExecutorCheckTest.CommandCheckDeliveredAndReconciled
> [ FAILED ] DefaultExecutorCheckTest.CommandCheckStatusChange
> [ FAILED ] DefaultExecutorCheckTest.CommandCheckSeesParentsEnv
> [ FAILED ] DefaultExecutorCheckTest.CommandCheckSharesWorkDirWithTask
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4d5dc13f363f5d8886983d7dd06a5cecc177c345 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 63d3cf80d8af6347c70fa6b7e2fd824faa0c7e3f 
>   src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce 
> 
> 
> Diff: https://reviews.apache.org/r/62187/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 62187: This patch is needed to triger Apache CI build.

2017-09-08 Thread Andrei Budnik 

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

Review request for mesos, Andrei Budnik and Alexander Rukletsov.


Repository: mesos


Description
---

I need to check potential security issue caused by creating unix
sockets in /tmp directory, which leads to failed tests:
[ FAILED ] CommandExecutorCheckTest.CommandCheckDeliveredAndReconciled
[ FAILED ] CommandExecutorCheckTest.CommandCheckStatusChange
[ FAILED ] DefaultExecutorCheckTest.CommandCheckDeliveredAndReconciled
[ FAILED ] DefaultExecutorCheckTest.CommandCheckStatusChange
[ FAILED ] DefaultExecutorCheckTest.CommandCheckSeesParentsEnv
[ FAILED ] DefaultExecutorCheckTest.CommandCheckSharesWorkDirWithTask


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
4d5dc13f363f5d8886983d7dd06a5cecc177c345 
  src/slave/containerizer/mesos/io/switchboard.cpp 
63d3cf80d8af6347c70fa6b7e2fd824faa0c7e3f 
  src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 62018: Updated logging::initialize to make flags optional.

2017-09-08 Thread Andrei Budnik 

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




src/logging/logging.cpp
Line 156 (original), 165 (patched)


What will be the value of `FLAGS_logtostderr` if `_flags` is `None`? Does 
it mean that messages via `LOG` won't be written to `stdout`/`stderr`?


- Andrei Budnik


On Sept. 6, 2017, 9:48 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62018/
> ---
> 
> (Updated Sept. 6, 2017, 9:48 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Thiw will be used to initialize logging in the main functions
> that do not use mesos::internal::logging::Flags.
> 
> 
> Diffs
> -
> 
>   src/logging/logging.hpp 0e448f25de6c66d772f5bc481fbaf4aa493fd8af 
>   src/logging/logging.cpp 70d66a5c396f709e8f27ad0d51315ed6d257f73b 
> 
> 
> Diff: https://reviews.apache.org/r/62018/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> $ make check
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

2017-09-08 Thread Andrei Budnik 

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




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


Should we move it one line before `testTCPConnect`?
It looks like initialization code which should go right after 
parsing/validating of flags, isn't it?



src/checks/tcp_connect.cpp
Line 138 (original), 140 (patched)


Consider moving it just after `Try load = 
flags.load(None(), argc, argv);` line.



src/cli/execute.cpp
Lines 919 (patched)


I would prefer passing `installFailureSignalHandler = true` here and 
everywhere for `logging::initialize` - it will help us a lot debugging them.



src/cli/execute.cpp
Line 956 (original), 957 (patched)


Do we need `return EXIT_FAILURE` here?


- Andrei Budnik


On Sept. 6, 2017, 9:57 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62037/
> ---
> 
> (Updated Sept. 6, 2017, 9:57 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change reduces the number of messages "WARNING: Logging
> before InitGoogleLogging() is written to STDERR".
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 3cb9077b9f39f69fdc152280e5b180b88e772648 
>   src/cli/execute.cpp e844fa46a43bdfb08246be929b7913a7997f18df 
>   src/cli/resolve.cpp b3cba87f61dbff0e99291775a4d0908bb35ce811 
>   src/examples/balloon_framework.cpp 1a97f56c3e217c58b408949306b92f19caa5f6c6 
>   src/examples/disk_full_framework.cpp 
> f9d5af5e45d5736581ce2c5395c741c995332136 
>   src/examples/dynamic_reservation_framework.cpp 
> bb6f58ba8fb87ae37d4ae971c192a204d5a656c5 
>   src/examples/long_lived_framework.cpp 
> 3de4a0280c41db5dcf656ac89cb4168060190108 
>   src/examples/no_executor_framework.cpp 
> 2ca240b4c998f0e33e7b09dbad9f4a7bfa0583cc 
>   src/examples/test_framework.cpp a6b38f02cc29682e3d3e1adf83baea061feda9e7 
>   src/examples/test_http_framework.cpp 
> 693dd47694678e7439f9d4ab0ff77b93950edf6e 
>   src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
>   src/slave/container_loggers/logrotate.cpp 
> 61484b18f4615e85925b26d999afb5ac3b7e32a5 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> fa0b29667258b6b6fb7381d8226b6dda0062629e 
>   src/usage/main.cpp 5ad4a3ba7959e4a9b3fa05f5a2f49b02f94a0cf0 
> 
> 
> Diff: https://reviews.apache.org/r/62037/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> make check
> ```
> 
> Had an issue with ExamplesTest.DiskFullFramework that is not related.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62147: Added a comment about master sending a checkpointed resources message.

2017-09-08 Thread Jan Schlicht 

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


Ship it!




Ship It!

- Jan Schlicht


On Sept. 7, 2017, 6:11 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62147/
> ---
> 
> (Updated Sept. 7, 2017, 6:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Michael Park, 
> and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a comment about master sending a checkpointed resources message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 14d94cfa0e96881f77dd6e0c3449fdc0613e36ce 
> 
> 
> Diff: https://reviews.apache.org/r/62147/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 60593: Added `network/ports` isolator recovery tests.

2017-09-08 Thread Qian Zhang 

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


Ship it!




Ship It!

- Qian Zhang


On Sept. 8, 2017, 5:57 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60593/
> ---
> 
> (Updated Sept. 8, 2017, 5:57 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for the `network/ports` isolator recovery by starting
> a task that is listening on a rogue port. We only configure the
> isolator when we restart the agent to simulate the case where a
> task only starts to misbehave after an agent recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60593/diff/16/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60496: Added socket checking to the network ports isolator.

2017-09-08 Thread Qian Zhang 

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


Ship it!




Ship It!

- Qian Zhang


On Sept. 8, 2017, 8:09 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> ---
> 
> (Updated Sept. 8, 2017, 8:09 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented ports resource restrictions in the network ports isolator.
> Periodically, scan for listening sockets and match them up to all
> the open sockets in the containers we are tracking in the network.
> Check any sockets we find against the ports resource and trigger a
> resource limitation if the port has not been allocated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60496/diff/18/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61536: Added network ports isolator socket utilities tests.

2017-09-08 Thread Qian Zhang 

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




src/tests/containerizer/ports_isolator_tests.cpp
Lines 51 (patched)


The test case name should always be in the form of `xxxTest` in Mesos, so I 
would suggest to rename `NetworkPortsIsolator` to 
`NetworkPortsIsolatorUtilityTest`.


- Qian Zhang


On Sept. 6, 2017, 12:36 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61536/
> ---
> 
> (Updated Sept. 6, 2017, 12:36 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some basic tests for the network ports isolator socket
> utilities. We test that we can enumerate our own sockets and
> use that to figure out what ports we are listening on.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900 
>   src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61536/diff/7/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

2017-09-08 Thread Qian Zhang 

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




src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 112 (original), 112 (patched)


Why do we need a `for` loop like this? I think the previous `while` loop is 
good enough.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 124 (original), 125 (patched)


This seems too strict, since here we are already going to return an error 
out, I think we can simply ignore the error of `closedir()`.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 140 (patched)


Ditto.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 144 (patched)


Better to change this to:
```
  if (closedir(dir) == -1) {
return ErrnoError("Failed to close directory '" + fdPath + "'");
  }
```
This is also aligned with how you handle the error of `opendir()` in the 
above.


- Qian Zhang


On Sept. 8, 2017, 5:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 8, 2017, 5:50 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented network ports isolator socket utilities to find the
> inode numbers of all the listening sockets, and the inodes of the
> open sockets for a process. Together, these utilities can tell us
> which sockets a process is listening on.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60495/diff/15/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>