Re: Review Request 71666: SSL Wrapper: Implemented send/recv and shutdown.

2019-11-05 Thread Mesos Reviewbot

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



Patch looks great!

Reviews applied: [71659, 71660, 71661, 71662, 71663, 71664, 71665, 71666]

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

- Mesos Reviewbot


On Nov. 6, 2019, 3:41 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> ---
> 
> (Updated Nov. 6, 2019, 3:41 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This completes a fully functional client-side SSL socket.
> 
> Needs a bit of cleanup and more error handling though.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71666/diff/4/
> 
> 
> Testing
> ---
> 
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
> 
> Running libprocess-tests yields:
> ```
> [  FAILED  ] SSLTest.ValidDowngrade
> [  FAILED  ] SSLTest.ValidDowngradeEachProtocol
> [  FAILED  ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = 
> "SSL"
> [  FAILED  ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 71666: SSL Wrapper: Implemented send/recv and shutdown.

2019-11-05 Thread Joseph Wu

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

(Updated Nov. 5, 2019, 7:41 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
Toenshoff.


Changes
---

Adjusted shutdown logic to avoid deadlocks with the 
ProcessManager/SocketManager.  (Basically, don't use process::loop here)


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


Repository: mesos


Description
---

This completes a fully functional client-side SSL socket.

Needs a bit of cleanup and more error handling though.


Diffs (updated)
-

  3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
  3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 


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

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


Testing (updated)
---

```
cmake --build . --target libprocess-tests
libprocess-tests
```

Running libprocess-tests yields:
```
[  FAILED  ] SSLTest.ValidDowngrade
[  FAILED  ] SSLTest.ValidDowngradeEachProtocol
[  FAILED  ] Encryption/NetSocketTest.EOFBeforeRecv/0, where GetParam() = "SSL"
[  FAILED  ] Encryption/NetSocketTest.EOFAfterRecv/0, where GetParam() = "SSL"
```


Thanks,

Joseph Wu



Re: Review Request 71666: SSL Wrapper: Implemented send/recv and shutdown.

2019-11-05 Thread Joseph Wu

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

(Updated Nov. 5, 2019, 5:57 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
Toenshoff.


Changes
---

Changed lambda captures of `this` to a weak pointer instead.


Summary (updated)
-

SSL Wrapper: Implemented send/recv and shutdown.


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


Repository: mesos


Description
---

This completes a fully functional client-side SSL socket.

Needs a bit of cleanup and more error handling though.


Diffs (updated)
-

  3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
  3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 


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

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


Testing (updated)
---

```
cmake --build . --target libprocess-tests
libprocess-tests --gtest_filter="SSLTest.*"
```

Running libprocess-tests yields:
```
[  FAILED  ] SSLTest.ValidDowngrade
[  FAILED  ] SSLTest.ValidDowngradeEachProtocol
[  FAILED  ] SSLTest.ShutdownThenSend
```


Thanks,

Joseph Wu



Re: Review Request 71665: SSL Wrapper: Implemented socket connection and handshake.

2019-11-05 Thread Joseph Wu

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

(Updated Nov. 5, 2019, 5:55 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
Toenshoff.


Changes
---

* Changed lambdas to hold weak pointers to the socket wrapper.
* Added an accept queue and loop to deal with potential connections that never 
complete handshaking.
* Checked for EOF in the handshake loop.


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


Repository: mesos


Description
---

This fills in some of the SSL socket implementation,
in particular the constructor, destructor, connect(),
and accept() methods.

Much of the setup and verification is taken verbatim from the
libevent socket implementation.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 
48860f8646d388685f0a60ad2a2f613b1f4be61a 
  3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
  3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 


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

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


Testing
---

cmake --build . --target libprocess-tests

Successfully connected to Google :D
With something like this:
```
  set_environment_variables({
{"LIBPROCESS_SSL_ENABLED", "true"},
{"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
{"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
  });

  Try client = Socket::create(SocketImpl::Kind::SSL);
  ASSERT_SOME(client);

  AWAIT_ASSERT_READY(client->connect(
  network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
  openssl::create_tls_client_config(None(;
```


Thanks,

Joseph Wu



Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass along new 'source' field.

2019-11-05 Thread Greg Mann

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




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


Should this be an Option? Since it isn't, it seems like we will set the 
`source` field to a default-constructed value in `_reserve()` when the operator 
does not specify that parameter. Would it be better to avoid setting the 
`source` field at all in the offer operation if it isn't found in the query 
parameters?


- Greg Mann


On Nov. 5, 2019, 1:25 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71695/
> ---
> 
> (Updated Nov. 5, 2019, 1:25 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9987 and MESOS-9989
> https://issues.apache.org/jira/browse/MESOS-9987
> https://issues.apache.org/jira/browse/MESOS-9989
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated 'Master::Http::_reserve()' to correctly set the new `source`
> field in the `Offer::Operation` created from operator API input.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da 
>   src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 
> 
> 
> Diff: https://reviews.apache.org/r/71695/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71722: Improved error reporting in 'Resources::pushReservation()'.

2019-11-05 Thread Mesos Reviewbot

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



Patch looks great!

Reviews applied: [71722]

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

- Mesos Reviewbot


On Nov. 5, 2019, 5:05 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71722/
> ---
> 
> (Updated Nov. 5, 2019, 5:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure to print the actual error message after an assertion
> failure in `Resources::pushReservations()`.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 
> 
> 
> Diff: https://reviews.apache.org/r/71722/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71700: Updated offer operation resource validation for reservation updates.

2019-11-05 Thread Mesos Reviewbot

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



Patch looks great!

Reviews applied: [71679, 71686, 71699, 71687, 71688, 71690, 71695, 71696, 71700]

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

- Mesos Reviewbot


On Oct. 29, 2019, 6:13 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71700/
> ---
> 
> (Updated Oct. 29, 2019, 6:13 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `validateAndUpgradeResources()` to also validate the `source`
> field in a reservation.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.cpp cbdad4b045ad957b2ab1fd09a6b9fb12bb4fb523 
> 
> 
> Diff: https://reviews.apache.org/r/71700/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71690: Added function to compute a common reservation ancestor.

2019-11-05 Thread Benno Evers

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

(Updated Nov. 5, 2019, 5:12 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Fixed a bug where the reservation ancestor was not properly computed.


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


Repository: mesos


Description
---

Added a new function `Resources::getReservationAncestor()` to compute
a common reservation ancestor between two `Resources`.


Diffs (updated)
-

  include/mesos/resources.hpp b8aef28e08f85c87bb78f25a64b0d7318f2727cc 
  src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 
  src/tests/resources_tests.cpp b5854656b7e9ce7af9e1d8ecad708066512d814f 


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

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


Testing
---


Thanks,

Benno Evers



Review Request 71722: Improved error reporting in 'Resources::pushReservation()'.

2019-11-05 Thread Benno Evers

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Make sure to print the actual error message after an assertion
failure in `Resources::pushReservations()`.


Diffs
-

  src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 


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


Testing
---


Thanks,

Benno Evers



Re: Review Request 71700: Updated offer operation resource validation for reservation updates.

2019-11-05 Thread Mesos Reviewbot

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



Patch looks great!

Reviews applied: [71679, 71686, 71699, 71687, 71688, 71690, 71695, 71696, 71700]

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

- Mesos Reviewbot


On Oct. 29, 2019, 6:13 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71700/
> ---
> 
> (Updated Oct. 29, 2019, 6:13 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `validateAndUpgradeResources()` to also validate the `source`
> field in a reservation.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.cpp cbdad4b045ad957b2ab1fd09a6b9fb12bb4fb523 
> 
> 
> Diff: https://reviews.apache.org/r/71700/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass along new 'source' field.

2019-11-05 Thread Benno Evers

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

(Updated Nov. 5, 2019, 1:25 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Incorporated fixups by @bbannier


Bugs: MESOS-9987 and MESOS-9989
https://issues.apache.org/jira/browse/MESOS-9987
https://issues.apache.org/jira/browse/MESOS-9989


Repository: mesos


Description
---

Updated 'Master::Http::_reserve()' to correctly set the new `source`
field in the `Offer::Operation` created from operator API input.


Diffs (updated)
-

  src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da 
  src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 


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

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


Testing
---


Thanks,

Benno Evers



Re: Review Request 71696: Updated validation of 'Reserve' call.

2019-11-05 Thread Benno Evers

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

(Updated Nov. 5, 2019, 1:25 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Incorporated fixups by @bbannier.


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


Repository: mesos


Description
---

Updated `master::validation::operation::validate(Reserve&)` to
account for the new `source` field in the reserve call, verifying
that `source` and `resources` have a common reservation ancestor.


Diffs (updated)
-

  src/master/validation.cpp a7ecefb8a1e186901301419feca75600d8de001b 


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

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


Testing
---


Thanks,

Benno Evers



Re: Review Request 71690: Added function to compute a common reservation ancestor.

2019-11-05 Thread Benno Evers

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

(Updated Nov. 5, 2019, 1:24 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Incorporated changes by @bbannier


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


Repository: mesos


Description
---

Added a new function `Resources::getReservationAncestor()` to compute
a common reservation ancestor between two `Resources`.


Diffs (updated)
-

  include/mesos/resources.hpp b8aef28e08f85c87bb78f25a64b0d7318f2727cc 
  src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 
  src/tests/resources_tests.cpp b5854656b7e9ce7af9e1d8ecad708066512d814f 


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

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


Testing
---


Thanks,

Benno Evers