Re: Review Request 71742: Mesos agent shouldn't respond pings if no master is registered

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71742]

Failed command: ['bash', '-c', "set -o pipefail; 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 2>&1 | tee 
build_71742"]

Error:
..
principal"},"source":{"id":"/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_3yGpKu/2GB-da412f9a-c645-426e-830f-36efccaf33ab","mount":{"root":"./csi/org.apache.mesos.csi.test/local/mounts"},"profile":"test","type":"MOUNT","vendor":"org.apache.mesos.csi.test.local"},"volume":{"container_path":"volume","mode":"RW"}},"name":"disk","provider_id":{"value":"4238df44-e758-45fa-a640-2b50ff3f9d9b"},"reservations":[{"role":"storage","type":"DYNAMIC"},{"principal":"test-principal","role":"storage/default-role","type":"DYNAMIC"}],"scalar":{"value":2048.0},"type":"SCALAR"}]'
I1120 06:06:59.865097 18662 sched.cpp:960] Rescinded offer 
a9e4592f-64ab-4112-9e88-7566facc2bed-O3
I1120 06:06:59.865196 18662 sched.cpp:971] Scheduler::offerRescinded took 
40528ns
I1120 06:06:59.865566 18648 hierarchical.cpp:1576] Recovered ports(allocated: 
storage/default-role):[31000-32000]; disk(allocated: 
storage/default-role)(reservations: 
[(DYNAMIC,storage),(DYNAMIC,storage/default-role,test-principal)])[MOUNT(org.apache.mesos.csi.test.local,/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_3yGpKu/2GB-da412f9a-c645-426e-830f-36efccaf33ab,test)]:2048;
 cpus(allocated: storage/default-role):2; mem(allocated: 
storage/default-role):1024; disk(allocated: storage/default-role):1024 (total: 
cpus:2; mem:1024; disk:1024; ports:[31000-32000]; disk(reservations: 
[(DYNAMIC,storage),(DYNAMIC,storage/default-role,test-principal)])[MOUNT(org.apache.mesos.csi.test.local,/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_3yGpKu/2GB-da412f9a-c645-426e-830f-36efccaf33ab,test)]:2048,
 offered or allocated: {}) on agent a9e4592f-64ab-4112-9e88-7566facc2bed-S0 
from framework a9e45
 92f-64ab-4112-9e88-7566facc2bed-
I1120 06:06:59.865661 18670 master.cpp:12794] Removing offer 
a9e4592f-64ab-4112-9e88-7566facc2bed-O3
I1120 06:06:59.866827 18648 hierarchical.cpp:1625] Framework 
a9e4592f-64ab-4112-9e88-7566facc2bed- filtered agent 
a9e4592f-64ab-4112-9e88-7566facc2bed-S0 for 5secs
I1120 06:06:59.869273 18651 master.cpp:12659] Sending operation '' (uuid: 
432806fd-224b-4139-aa48-1abc35ccafeb) to agent 
a9e4592f-64ab-4112-9e88-7566facc2bed-S0 at slave(1250)@172.17.0.2:42302 
(7ccc8b834dd0)
I1120 06:06:59.869983 18649 slave.cpp:4352] Ignoring new checkpointed resources 
and operations identical to the current version
I1120 06:06:59.873303 18671 provider.cpp:498] Received APPLY_OPERATION event
I1120 06:06:59.873347 18671 provider.cpp:1351] Received CREATE operation '' 
(uuid: 432806fd-224b-4139-aa48-1abc35ccafeb)
I1120 06:06:59.879556 18657 master.cpp:6482] Processing REVIVE call for 
framework a9e4592f-64ab-4112-9e88-7566facc2bed- (default) at 
scheduler-5eaf4119-5232-42ab-89d3-bcfa3995da6d@172.17.0.2:42302
I1120 06:06:59.880115 18657 hierarchical.cpp:1721] Unsuppressed offers and 
cleared filters for roles { storage/default-role } of framework 
a9e4592f-64ab-4112-9e88-7566facc2bed-
I1120 06:06:59.881371 18657 hierarchical.cpp:1853] Performed allocation for 1 
agents in 1.047256ms
I1120 06:06:59.881721 18657 hierarchical.cpp:1853] Performed allocation for 1 
agents in 151686ns
I1120 06:06:59.881968 18670 master.cpp:10497] Sending offers [ 
a9e4592f-64ab-4112-9e88-7566facc2bed-O4 ] to framework 
a9e4592f-64ab-4112-9e88-7566facc2bed- (default) at 
scheduler-5eaf4119-5232-42ab-89d3-bcfa3995da6d@172.17.0.2:42302
I1120 06:06:59.882639 18655 sched.cpp:934] Scheduler::resourceOffers took 
101940ns
I1120 06:06:59.893764 18661 http.cpp:1115] HTTP POST for 
/slave(1250)/api/v1/resource_provider from 172.17.0.2:44786
I1120 06:06:59.894723 18649 slave.cpp:8495] Handling resource provider message 
'UPDATE_OPERATION_STATUS: (uuid: 886bf308-26b8-4c87-a3d8-aaf636839fab) for 
framework  (latest state: OPERATION_FINISHED, status update state: 
OPERATION_FINISHED)'
I1120 06:06:59.894937 18649 slave.cpp:8948] Updating the state of operation 
with no ID (uuid: 886bf308-26b8-4c87-a3d8-aaf636839fab) for an operation API 
call (latest state: OPERATION_FINISHED, status update state: OPERATION_FINISHED)
I1120 06:06:59.894990 18649 slave.cpp:8702] Forwarding status update of 
operation with no ID (operation_uuid: 886bf308-26b8-4c87-a3d8-aaf636839fab) for 
an operator API call
I1120 06:06:59.895336 18664 master.cpp:12311] Updating the state of operation 

Re: Review Request 71790: SSL Socket: Implemented sendfile.

[Benjamin Mahler]

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



Are you aware of kTLS?

E.g. https://blog.filippo.io/playing-with-kernel-tls-in-linux-4-13-and-go/

- Benjamin Mahler


On Nov. 20, 2019, 12:29 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71790/
> ---
> 
> (Updated Nov. 20, 2019, 12:29 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 implements the SSL socket's sendfile method, which must read
> the file (unlike the zero-copy os::sendfile).
> 
> This also moves a test exercising sendfile from process_tests.cpp
> into http_tests.cpp and parameterizes it for SSL and non-SSL.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 1433f3d09a72133fab1441be53562d508bc01682 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 05dc5ec2fdc74a989689e4378bef775bcf2b7a87 
> 
> 
> Diff: https://reviews.apache.org/r/71790/diff/2/
> 
> 
> Testing
> ---
> 
> cmake --build . --target libprocess-tests
> libprocess-tests
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71755: Improved performance of v1 operator API GetMetrics call.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71747, 71748, 71749, 71750, 71751, 71752, 71753, 71754, 71755]

Failed command: ['bash', '-c', "set -o pipefail; 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 2>&1 | tee 
build_71755"]

Error:
..
mesos-1.10.0/_inst/share/mesos\" 
-DPKGMODULEDIR=\"/mesos/mesos-1.10.0/_inst/lib/mesos/modules\" -I../../include 
-I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 
-I../3rdparty/libarchive-3.3.2/libarchive/ -I../../3rdparty/libprocess/include 
-I../3rdparty/nvml-352.79 -I../3rdparty/picojson-1.3.0 
-I../3rdparty/protobuf-3.5.0/src -I../3rdparty/rapidjson-1.1.0/include 
-I../../3rdparty/stout/include -I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated -I/usr/include/subversion-1 
-I/usr/include/apr-1 -I/usr/include/apr-1.0 -pthread -Wall -Wsign-compare 
-Wformat-security -fstack-protector -fPIC -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c ../../src/logging/flags.cpp  -fPIC -DPIC -o 
logging/.libs/libmesos_no_3rdparty_la-flags.o
/bin/bash ../libtool  --tag=CXX   --mode=compile g++ -DPACKAGE_NAME=\"mesos\" 
-DPACKAGE_TARNAME=\"mesos\" -DPACKAGE_VERSION=\"1.10.0\" 
-DPACKAGE_STRING=\"mesos\ 1.10.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" 
-DPACKAGE=\"mesos\" -DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 
-DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 
-DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 
-DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../src   -Werror -DLIBDIR=\"/mesos/mesos-1.10.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/mesos/mesos-1.10.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/mesos/mesos-1.
 10.0/_inst/share/mesos\" 
-DPKGMODULEDIR=\"/mesos/mesos-1.10.0/_inst/lib/mesos/modules\" -I../../include 
-I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 
-I../3rdparty/libarchive-3.3.2/libarchive/ -I../../3rdparty/libprocess/include  
-I../3rdparty/nvml-352.79 -I../3rdparty/picojson-1.3.0 
-I../3rdparty/protobuf-3.5.0/src -I../3rdparty/rapidjson-1.1.0/include 
-I../../3rdparty/stout/include -I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated -I/usr/include/subversion-1 
-I/usr/include/apr-1 -I/usr/include/apr-1.0  -pthread -Wall 
-Wsign-compare -Wformat-security -fstack-protector -fPIC -fPIE -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c -o 
logging/libmesos_no_3rdparty_la-logging.lo `test -f 'logging/logging.cpp' || 
echo '../../src/'`logging/logging.cpp
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" "-DPACKAGE_STRING=\"mesos 1.10.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DHAVE_LIBSASL2=1 
-DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 
-DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" 
-DMESOS_HAS_PYTHON=1 -I. -I../../src -Werror 
-DLIBDIR=\"/mesos/mesos-1.10.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/mesos/mesos-1.10.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/mesos/mesos-1.10.0/_inst/share/mesos\" -DPKGM
 ODULEDIR=\"/mesos/mesos-1.10.0/_inst/lib/mesos/modules\" -I../../include 
-I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 

Re: Review Request 71787: Added end-to-end test for reservation update with persistent volume.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71785, 71787]

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. 19, 2019, 8:53 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71787/
> ---
> 
> (Updated Nov. 19, 2019, 8:53 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9992
> https://issues.apache.org/jira/browse/MESOS-9992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new test that verifies that the reservation for a
> persistent disk volume can be updated after it has been
> created.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> bddc9467c5b9fe6cdcbd84f1b110356a43b59ba0 
> 
> 
> Diff: https://reviews.apache.org/r/71787/diff/1/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests --gtest_filter="*ReservationUpdate*"`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

[Joseph Wu]

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

(Updated Nov. 19, 2019, 4:29 p.m.)


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


Changes
---

* Added a "compute_thread" so that calls to `SSL_read` and `SSL_write` can be 
spread onto (many) libprocess worker threads rather than the (one) event loop 
thread.
* Changed shutdown to happen asynchronously but accept dirty shutdowns.
* Guarded against new send/recv calls after 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/6/

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


Testing (updated)
---

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

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


Thanks,

Joseph Wu

Re: Review Request 71790: SSL Socket: Implemented sendfile.

[Joseph Wu]

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

(Updated Nov. 19, 2019, 4:29 p.m.)


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


Changes
---

Rebase on top of compute_thread and shutdown changes.


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


Repository: mesos


Description
---

This implements the SSL socket's sendfile method, which must read
the file (unlike the zero-copy os::sendfile).

This also moves a test exercising sendfile from process_tests.cpp
into http_tests.cpp and parameterizes it for SSL and non-SSL.


Diffs (updated)
-

  3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
1433f3d09a72133fab1441be53562d508bc01682 
  3rdparty/libprocess/src/tests/process_tests.cpp 
05dc5ec2fdc74a989689e4378bef775bcf2b7a87 


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

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


Testing
---

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


Thanks,

Joseph Wu

Review Request 71790: SSL Socket: Implemented sendfile.

[Joseph Wu]

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

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 implements the SSL socket's sendfile method, which must read
the file (unlike the zero-copy os::sendfile).

This also moves a test exercising sendfile from process_tests.cpp
into http_tests.cpp and parameterizes it for SSL and non-SSL.


Diffs
-

  3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
1433f3d09a72133fab1441be53562d508bc01682 
  3rdparty/libprocess/src/tests/process_tests.cpp 
05dc5ec2fdc74a989689e4378bef775bcf2b7a87 


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


Testing
---

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


Thanks,

Joseph Wu

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

[Mesos Reviewbot]

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



Bad review!

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

Error:
2019-11-19 22:48:17 URL:https://reviews.apache.org/r/71665/diff/raw/ 
[16358/16358] -> "71665.patch" [1]
error: patch failed: 3rdparty/libprocess/src/ssl/socket_wrapper.cpp:30
error: 3rdparty/libprocess/src/ssl/socket_wrapper.cpp: patch does not apply

- Mesos Reviewbot


On Nov. 11, 2019, 7:41 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> ---
> 
> (Updated Nov. 11, 2019, 7:41 p.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/5/
> 
> 
> 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 71742: Mesos agent shouldn't respond pings if no master is registered

[Xudong Ni via Review Board]

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

(Updated Nov. 19, 2019, 10:39 p.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Addressed the comments except the test which will be updated later


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


Repository: mesos


Description
---

In the case agents lost ZooKeeper connections and resetting its
master to none and beginning to dropping control messages from the
master, agent should not respond pings from master.


Diffs (updated)
-

  src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 


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

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


Testing
---

==] 2322 tests from 222 test cases ran. (1038166 ms total)
[  PASSED  ] 2321 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery, where TypeParam = 
mesos::internal::slave::MesosContainerizer

This failed test verifies that the agent responds to pings from the master 
while the agent is performing recovery, this PR will break this scenario.


Thanks,

Xudong Ni

Re: Review Request 71750: Improved performance of v1 operator API GetAgents call.

[Benjamin Mahler]

> On Nov. 19, 2019, 6:12 p.m., Andrei Sekretenko wrote:
> > src/master/http.cpp
> > Lines 2242 (patched)
> > 
> >
> > Did you consider allocating the `agent` temporaries outside of the 
> > loops? In my experience, protobuf construction used to be much more 
> > expensive than modification.
> > 
> > (Tried this, observed a ~10% speedup for `v1 'master::call::GetState' 
> > application/x-protobuf` in my setup.)
> 
> Benjamin Mahler wrote:
> Hm.. that's interesting, could certainly look into this for the GetAgents 
> and GetFrameworks code which both use temporaries. The temporaries aren't 
> necessary though, we could eliminate the need to copy entirely, but 
> allocating them outside the loops seems like an easier win.
> 
> Unfortunately the most expensive part in practice (non-pending tasks) are 
> not using temporaries like this. I'll look into a patch at the end of this 
> chain to allocate outside the loops.
> 
> Benjamin Mahler wrote:
> > (Tried this, observed a ~10% speedup for v1 'master::call::GetState' 
> application/x-protobuf in my setup.)
> 
> Curious to see your setup (did you just use a lot of agents instead of a 
> lot of tasks?) and your diff.

Also did you run with optimizations on? How many runs did you do? Because 
there's quite a high variance. Just did 10 runs of 
AgentFrameworkTaskCountContentType/MasterStateQuery_BENCHMARK_Test.GetState/3:

before: median 5.29 from [4.41, 4.51, 5.07, 5.10, 5.27, 5.32, 5.49, 5.51, 5.70, 
6.46]
after:  median 5.11 from [4.68, 4.79, 4.80, 4.84, 5.01, 5.21, 5.25, 5.42, 6.42, 
7.43]

So based on medians it's 96.6% of the time without the loop change?

With the following diff applied to this chain to produce 'after':

```
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 0ca25b6d7..c77788cdf 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1480,6 +1480,7 @@ string Master::Http::serializeGetFrameworks(
   // expect a large number of pending tasks, we currently don't
   // bother with the more efficient approach.
 
+  mesos::master::Response::GetFrameworks::Framework f;
   foreachvalue (const Framework* framework,
 master->frameworks.registered) {
 // Skip unauthorized frameworks.
@@ -1487,7 +1488,7 @@ string Master::Http::serializeGetFrameworks(
   continue;
 }
 
-mesos::master::Response::GetFrameworks::Framework f = model(*framework);
+f = model(*framework);
 writer.WriteTag(
 WireFormatLite::MakeTag(
 mesos::master::Response::GetFrameworks
@@ -1504,7 +1505,7 @@ string Master::Http::serializeGetFrameworks(
   continue;
 }
 
-mesos::master::Response::GetFrameworks::Framework f = model(*framework);
+f = model(*framework);
 writer.WriteTag(
 WireFormatLite::MakeTag(
 mesos::master::Response::GetFrameworks
@@ -2970,15 +2971,15 @@ string Master::Http::serializeGetAgents(
   google::protobuf::io::StringOutputStream stream();
   google::protobuf::io::CodedOutputStream writer();
 
+  mesos::master::Response::GetAgents::Agent agent;
   foreachvalue (const Slave* slave, master->slaves.registered) {
 // TODO(bmahler): Consider not constructing the temporary
 // agent object and instead serialize directly.
-mesos::master::Response::GetAgents::Agent agent =
-  protobuf::master::event::createAgentResponse(
-  *slave,
-  master->slaves.draining.get(slave->id),
-  master->slaves.deactivated.contains(slave->id),
-  approvers);
+agent = protobuf::master::event::createAgentResponse(
+*slave,
+master->slaves.draining.get(slave->id),
+master->slaves.deactivated.contains(slave->id),
+approvers);
 
 writer.WriteTag(
 WireFormatLite::MakeTag(
@@ -2988,14 +2989,15 @@ string Master::Http::serializeGetAgents(
 agent.SerializeToCodedStream();
   }
 
+  SlaveInfo recoveredAgent;
   foreachvalue (const SlaveInfo& slaveInfo, master->slaves.recovered) {
 // TODO(bmahler): Consider not constructing the temporary
 // SlaveInfo object and instead serialize directly.
-SlaveInfo agent = slaveInfo;
-agent.clear_resources();
+recoveredAgent = slaveInfo;
+recoveredAgent.clear_resources();
 foreach (const Resource& resource, slaveInfo.resources()) {
   if (approvers->approved(resource)) {
-*agent.add_resources() = resource;
+*recoveredAgent.add_resources() = resource;
   }
 }
 
@@ -3003,8 +3005,8 @@ string Master::Http::serializeGetAgents(
 WireFormatLite::MakeTag(
 mesos::master::Response::GetAgents::kRecoveredAgentsFieldNumber,
 WireFormatLite::WIRETYPE_LENGTH_DELIMITED));
-writer.WriteVarint32(agent.ByteSizeLong());
-agent.SerializeToCodedStream();
+writer.WriteVarint32(recoveredAgent.ByteSizeLong());
+recoveredAgent.SerializeToCodedStream();
   }
 
   // 

Re: Review Request 71664: SSL Wrapper: Implemented BIO for SSL socket wrapper.

[Joseph Wu]

> On Nov. 19, 2019, 5:28 a.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp
> > Lines 83 (patched)
> > 
> >
> > It seems like `synchronized()` for atomic flags is implemented as a 
> > busy wait, was this choice intentional?

Yes, the logic of this SSL socket is expected to run on multiple threads (the 
creator of the socket's thread and the event loop thread, and [soon] libprocess 
worker threads), so we need some guards here.


- Joseph


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


On Nov. 19, 2019, 12:16 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71664/
> ---
> 
> (Updated Nov. 19, 2019, 12:16 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This implements the OpenSSL basic I/O abstraction based on the
> libprocess event loop.  This BIO wraps a socket and handles the
> reading/writing, using io::read and io::write.
> 
> This BIO can be passed into an SSL context to enable usage of
> SSL translation functions like SSL_read and SSL_write.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71664/diff/3/
> 
> 
> Testing
> ---
> 
> cmake --build . --target process
> 
> A tiny bit of testing next patch.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71664: SSL Wrapper: Implemented BIO for SSL socket wrapper.

[Joseph Wu]

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

(Updated Nov. 19, 2019, 12:16 p.m.)


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


Changes
---

Removed blocking `BIO_flush`, and addressed comments about BIO_METHOD creation.


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


Repository: mesos


Description
---

This implements the OpenSSL basic I/O abstraction based on the
libprocess event loop.  This BIO wraps a socket and handles the
reading/writing, using io::read and io::write.

This BIO can be passed into an SSL context to enable usage of
SSL translation functions like SSL_read and SSL_write.


Diffs (updated)
-

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


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

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


Testing
---

cmake --build . --target process

A tiny bit of testing next patch.


Thanks,

Joseph Wu

Re: Review Request 71750: Improved performance of v1 operator API GetAgents call.

[Benjamin Mahler]

> On Nov. 19, 2019, 6:12 p.m., Andrei Sekretenko wrote:
> > src/master/http.cpp
> > Lines 2242 (patched)
> > 
> >
> > Did you consider allocating the `agent` temporaries outside of the 
> > loops? In my experience, protobuf construction used to be much more 
> > expensive than modification.
> > 
> > (Tried this, observed a ~10% speedup for `v1 'master::call::GetState' 
> > application/x-protobuf` in my setup.)
> 
> Benjamin Mahler wrote:
> Hm.. that's interesting, could certainly look into this for the GetAgents 
> and GetFrameworks code which both use temporaries. The temporaries aren't 
> necessary though, we could eliminate the need to copy entirely, but 
> allocating them outside the loops seems like an easier win.
> 
> Unfortunately the most expensive part in practice (non-pending tasks) are 
> not using temporaries like this. I'll look into a patch at the end of this 
> chain to allocate outside the loops.

> (Tried this, observed a ~10% speedup for v1 'master::call::GetState' 
> application/x-protobuf in my setup.)

Curious to see your setup (did you just use a lot of agents instead of a lot of 
tasks?) and your diff.


- Benjamin


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


On Nov. 12, 2019, 12:03 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71750/
> ---
> 
> (Updated Nov. 12, 2019, 12:03 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the handling to serialize directly to protobuf or json
> from the in-memory v0 state, bypassing expensive intermediate
> serialization / de-serialization / object construction / object
> destruction.
> 
> This initial patch shows the approach that will be used for the
> other expensive calls. Note that this type of manual writing is
> more brittle and complex, but it can be mostly eliminated if we
> keep an up-to-date v1 GetState in memory in the future.
> 
> When this approach is applied fully to GetState, it leads to the
> following improvement:
> 
> Before:
> v0 '/state' response took 6.55 secs
> v1 'GetState' application/x-protobuf response took 24.08 secs
> v1 'GetState' application/json response took 22.76 secs
> 
> After:
> v0 '/state' response took 8.00 secs
> v1 'GetState' application/x-protobuf response took 5.73 secs
> v1 'GetState' application/json response took 9.62 secs
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da 
>   src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 
> 
> 
> Diff: https://reviews.apache.org/r/71750/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71750: Improved performance of v1 operator API GetAgents call.

[Benjamin Mahler]

> On Nov. 19, 2019, 6:12 p.m., Andrei Sekretenko wrote:
> > src/master/http.cpp
> > Lines 2242 (patched)
> > 
> >
> > Did you consider allocating the `agent` temporaries outside of the 
> > loops? In my experience, protobuf construction used to be much more 
> > expensive than modification.
> > 
> > (Tried this, observed a ~10% speedup for `v1 'master::call::GetState' 
> > application/x-protobuf` in my setup.)

Hm.. that's interesting, could certainly look into this for the GetAgents and 
GetFrameworks code which both use temporaries. The temporaries aren't necessary 
though, we could eliminate the need to copy entirely, but allocating them 
outside the loops seems like an easier win.

Unfortunately the most expensive part in practice (non-pending tasks) are not 
using temporaries like this. I'll look into a patch at the end of this chain to 
allocate outside the loops.


> On Nov. 19, 2019, 6:12 p.m., Andrei Sekretenko wrote:
> > src/master/http.cpp
> > Lines 2257-2263 (patched)
> > 
> >
> > There is a certain similarity between serializeGetAgents and 
> > jsonifyGetAgents which probably can be factored out... 
> > 
> > However, it might make more sense to leave this as is for now, and 
> > refactor it after we get to getTasks/Executors/Frameworks.

Yeah, the same is true for the other serialization paths which I just 
published. I debated refactoring in front or behind, and I think I'll go with 
behind, so that these patches just duplicate the common parts for now.


- Benjamin


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


On Nov. 12, 2019, 12:03 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71750/
> ---
> 
> (Updated Nov. 12, 2019, 12:03 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the handling to serialize directly to protobuf or json
> from the in-memory v0 state, bypassing expensive intermediate
> serialization / de-serialization / object construction / object
> destruction.
> 
> This initial patch shows the approach that will be used for the
> other expensive calls. Note that this type of manual writing is
> more brittle and complex, but it can be mostly eliminated if we
> keep an up-to-date v1 GetState in memory in the future.
> 
> When this approach is applied fully to GetState, it leads to the
> following improvement:
> 
> Before:
> v0 '/state' response took 6.55 secs
> v1 'GetState' application/x-protobuf response took 24.08 secs
> v1 'GetState' application/json response took 22.76 secs
> 
> After:
> v0 '/state' response took 8.00 secs
> v1 'GetState' application/x-protobuf response took 5.73 secs
> v1 'GetState' application/json response took 9.62 secs
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da 
>   src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 
> 
> 
> Diff: https://reviews.apache.org/r/71750/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 71752: Improved performance of v1 operator API GetExecutors call.

[Benjamin Mahler]

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

Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.


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


Repository: mesos


Description
---

This follow the same approach used in the GetAgents call;
serializing directly to protobuf or json from the in-memory
v0 state.


Diffs
-

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


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


Testing
---


Thanks,

Benjamin Mahler

Review Request 71751: Improved performance of v1 operator API GetFrameworks call.

[Benjamin Mahler]

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

Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.


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


Repository: mesos


Description
---

This follow the same approach used in the GetAgents call;
serializing directly to protobuf or json from the in-memory
v0 state.


Diffs
-

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


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


Testing
---


Thanks,

Benjamin Mahler

Review Request 71754: Improved performance of v1 operator API GetState call.

[Benjamin Mahler]

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

Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.


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


Repository: mesos


Description
---

This follow the same approach used in the GetAgents call;
serializing directly to protobuf or json from the in-memory
v0 state.


Diffs
-

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


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


Testing
---


Thanks,

Benjamin Mahler

Review Request 71753: Improved performance of v1 operator API GetTasks call.

[Benjamin Mahler]

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

Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.


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


Repository: mesos


Description
---

This follow the same approach used in the GetAgents call;
serializing directly to protobuf or json from the in-memory
v0 state.


Diffs
-

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


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


Testing
---


Thanks,

Benjamin Mahler

Review Request 71755: Improved performance of v1 operator API GetMetrics call.

[Benjamin Mahler]

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

Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.


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


Repository: mesos


Description
---

This follow the same approach used in the GetAgents call;
serializing directly to protobuf or json from the in-memory
v0 state.


Diffs
-

  src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da 


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


Testing
---


Thanks,

Benjamin Mahler

Re: Review Request 71659: SSL Wrapper: Allowed SSL without libevent.

[Joseph Wu]

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

(Updated Nov. 19, 2019, 11:32 a.m.)


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


Changes
---

Updated the `Dependencies` portion of the SSL doc, in addition to the other 
changes.  Addressed a comment too.


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


Repository: mesos


Description
---

This removes the configure-time check on having both
ENABLE_SSL and ENABLE_LIBEVENT set to true in order to have
SSL sockets.  The subsequent commits will add SSL support
to the poll socket class as a wrapper.

This also updates the related documentation for SSL,
including on Windows.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 62cb23e81abb2c7e4e8e13c83b45afb98708bd30 
  configure.ac f274f34e6982beb0a5a683526f7eb9c4ea744e40 
  docs/configuration/autotools.md 55a5de83729271b043bd1b8de67798ab52b195c6 
  docs/ssl.md f6beb4211f0c17fdaa4b52425cec405c9b6a7d56 
  docs/windows.md 35b12ddedec6f85b75adefa80d38fa439cd6b2d3 


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

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


Testing
---

Requires a libprocess build change too (next patch).


Thanks,

Joseph Wu

Re: Review Request 71750: Improved performance of v1 operator API GetAgents call.

[Andrei Sekretenko]

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




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


Did you consider allocating the `agent` temporaries outside of the loops? 
In my experience, protobuf construction used to be much more expensive than 
modification.

(Tried this, observed a ~10% speedup for `v1 'master::call::GetState' 
application/x-protobuf` in my setup.)



src/master/http.cpp
Lines 2257-2263 (patched)


There is a certain similarity between serializeGetAgents and 
jsonifyGetAgents which probably can be factored out... 

However, it might make more sense to leave this as is for now, and refactor 
it after we get to getTasks/Executors/Frameworks.


- Andrei Sekretenko


On Nov. 12, 2019, 12:03 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71750/
> ---
> 
> (Updated Nov. 12, 2019, 12:03 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the handling to serialize directly to protobuf or json
> from the in-memory v0 state, bypassing expensive intermediate
> serialization / de-serialization / object construction / object
> destruction.
> 
> This initial patch shows the approach that will be used for the
> other expensive calls. Note that this type of manual writing is
> more brittle and complex, but it can be mostly eliminated if we
> keep an up-to-date v1 GetState in memory in the future.
> 
> When this approach is applied fully to GetState, it leads to the
> following improvement:
> 
> Before:
> v0 '/state' response took 6.55 secs
> v1 'GetState' application/x-protobuf response took 24.08 secs
> v1 'GetState' application/json response took 22.76 secs
> 
> After:
> v0 '/state' response took 8.00 secs
> v1 'GetState' application/x-protobuf response took 5.73 secs
> v1 'GetState' application/json response took 9.62 secs
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da 
>   src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 
> 
> 
> Diff: https://reviews.apache.org/r/71750/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71749: Added a test for AsV1Protobuf.

[Andrei Sekretenko]

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


Fix it, then Ship it!





src/tests/common/http_tests.cpp
Lines 269 (patched)


How about covering nested MESSAGE (repeated and map/non-repeated) and 
repeated ENUM? (see my comment in r71748)

Not sure if we have all of those in our protobufs (adding a new `.proto` 
file for this test would be an overkill IMO), but at least we have a 
repeated-message-inside-nested with a `slave_id` in `scheduler::Call` 
(`Call::ReconcileOperations`).


- Andrei Sekretenko


On Nov. 11, 2019, 11:59 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71749/
> ---
> 
> (Updated Nov. 11, 2019, 11:59 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for AsV1Protobuf.
> 
> 
> Diffs
> -
> 
>   src/tests/common/http_tests.cpp 51c809d70d364530b4c7845f9382610c2eeef366 
> 
> 
> Diff: https://reviews.apache.org/r/71749/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71748: Support jsonifying v0 protobuf to v1 protobuf.

[Andrei Sekretenko]

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


Fix it, then Ship it!





src/common/http.cpp
Lines 177 (patched)


Is there any reason not to file a ticket for that and link it here?

This patch unblocks a way to usable V1, but is *really*  brittle (for 
example, I had to run diff on several pairs of `.proto`s and two C++ excerpts 
to review this). We must want to replace it with a permanent solution ;)



src/common/http.cpp
Lines 253 (patched)


There are at least three differences from the stout version that seem to be 
not covered by any Mesos test, including the new one you are adding in r71749.

How about covering this one (and also repeated ENUM + map/singular MESSAGE 
below) in r71749? 

P.S. I'm quite surprised to see that our V1 API coverage is that poor... 
probably this only affects JSON?


- Andrei Sekretenko


On Nov. 11, 2019, 11:59 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71748/
> ---
> 
> (Updated Nov. 11, 2019, 11:59 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to jsonify a v0 protobuf directly to a v1 protobuf
> efficiently, with no need to `evolve()` the message (which is rather
> expensive).
> 
> The way this works is by converting all "slave" and "SLAVE" strings
> in fields and enum values, respectively, to "agent" and "AGENT".
> 
> Our current v0 to v1 conversion for the v1 operator API simply
> serializes the v0 message and de-serializes into a v1 message, which
> means all field tags and message structures are the same, except
> for field names. The only difference with field names is the use
> of "agent" in place of "slave".
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 062586c0a5a339c7e63c89a3a893ae015d3fd26e 
>   src/common/http.cpp b79074f823d3bce2a15736c0ef4739ad13db8d9c 
> 
> 
> Diff: https://reviews.apache.org/r/71748/diff/1/
> 
> 
> Testing
> ---
> 
> Added a test in the subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 71787: Added end-to-end test for reservation update with persistent volume.

[Benno Evers]

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

Added a new test that verifies that the reservation for a
persistent disk volume can be updated after it has been
created.


Diffs
-

  src/tests/persistent_volume_endpoints_tests.cpp 
bddc9467c5b9fe6cdcbd84f1b110356a43b59ba0 


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


Testing
---

`./src/mesos-tests --gtest_filter="*ReservationUpdate*"`


Thanks,

Benno Evers

Review Request 71785: Removed check when validating reservations.

[Benno Evers]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Removed a validation that was checking that `RESERVE` operations
did not contain disk resources, based on the theory that disk
resources must already be reserved.

This assumption does not hold up in the presence of reservation
updates, so it is removed.


Diffs
-

  src/master/validation.cpp c5fbbdd55e4a217c756e2ece3f8fd6ed5e88048f 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 71659: SSL Wrapper: Allowed SSL without libevent.

[Benno Evers]

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


Fix it, then Ship it!





docs/ssl.md
Lines 12 (patched)


s/encypts/encrypts/

Also, I know it isn't your sentence, but "the communication that Mesos uses 
for communication" doesn't sound entirely correct either.


- Benno Evers


On Nov. 13, 2019, 7:01 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71659/
> ---
> 
> (Updated Nov. 13, 2019, 7:01 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes the configure-time check on having both
> ENABLE_SSL and ENABLE_LIBEVENT set to true in order to have
> SSL sockets.  The subsequent commits will add SSL support
> to the poll socket class as a wrapper.
> 
> This also updates the related documentation for SSL,
> including on Windows.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 62cb23e81abb2c7e4e8e13c83b45afb98708bd30 
>   configure.ac f274f34e6982beb0a5a683526f7eb9c4ea744e40 
>   docs/configuration/autotools.md 55a5de83729271b043bd1b8de67798ab52b195c6 
>   docs/ssl.md f6beb4211f0c17fdaa4b52425cec405c9b6a7d56 
>   docs/windows.md 35b12ddedec6f85b75adefa80d38fa439cd6b2d3 
> 
> 
> Diff: https://reviews.apache.org/r/71659/diff/2/
> 
> 
> Testing
> ---
> 
> Requires a libprocess build change too (next patch).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71741: Updated operator API documention to use rereservation format.

[Benjamin Bannier]

> On Nov. 18, 2019, 5:41 p.m., Benno Evers wrote:
> > I'm not sure that this example does much to explain the semantics of the 
> > `RESERVE_RESOURCES` call to unsuspecting readers: The example you modify 
> > will behave exactly the same whether the `source` field is present or not.
> > 
> > At the very least, `source` should be formally introduced in the 
> > description of the `RESERVE_RESOURCES` call at the beginning of the 
> > section. Ideally, we'd have a first example without `source` (i.e. the one 
> > we had before) and then a second example using `source` to do an actual 
> > reservation update. (In your commit message you use the word 
> > `rereservation`, do we use that anywhere else?)

I updated the documentation and examples. I went with two examples (covering 
both "simple reservation" and re-reservation). I did however in both cases 
specify `source` as IMO it is the cleaner API.


- Benjamin


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


On Nov. 8, 2019, 2:13 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71741/
> ---
> 
> (Updated Nov. 8, 2019, 2:13 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9993
> https://issues.apache.org/jira/browse/MESOS-9993
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated operator API documention to use rereservation format.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c 
> 
> 
> Diff: https://reviews.apache.org/r/71741/diff/1/
> 
> 
> Testing
> ---
> 
> Previewed in generated site
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71747: Eliminate unnecessary body copying in http::Response constructor.

[Andrei Sekretenko]

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


Ship it!




Ship It!

- Andrei Sekretenko


On Nov. 11, 2019, 11:39 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71747/
> ---
> 
> (Updated Nov. 11, 2019, 11:39 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the caller to pass an rvalue reference to the body,
> without it being copied.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> a0065db12b0c689404d8d5caa7021017bb4aa4d8 
> 
> 
> Diff: https://reviews.apache.org/r/71747/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.

[Benno Evers]

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




3rdparty/libprocess/src/CMakeLists.txt
Lines 23 (patched)


I'm not sure I completely understand the rationale for the exclusivity here 
(and elsewhere in the review). Intuitively, I'd expect

```
OpenSSL >= 1.1 found =>  Enable SocketWrapper
Libevent enabled => Enable LibeventSSLSocket
--enable-ssl => Require either of the above to be enabled
```

In particular it doesn't seem to be too far-fetched that someone might want 
to use libevent but not the libevent ssl socket in the future, due to bugs like 
MESOS-9867.



3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 24 (patched)


Why is this derived from `PollSocketImpl`, rather than just from 
`SocketImpl`?



3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 25 (patched)


This is mostly bike-shedding, so feel free to drop this issue if you 
disagree, but I'm not a huge fan of the name `SSLSocketWrapper`:

First, the name would suggest to me that the constructor argument `int_fd 
s` is already a SSL socket that is being wrapped by this class.

Second, the class doesn *wrap* a Socket, it *is* a SocketImpl.

Finally, regarding the other implementations of `SocketImpl`, it seems like 
`OpenSSLSocketImpl` would be a bit more consistent.


- Benno Evers


On Nov. 13, 2019, 7:03 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> ---
> 
> (Updated Nov. 13, 2019, 7:03 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
> 
> The new class is used whenever SSL is enabled, but Libevent is not.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 641251af5631e9b3928dfb282cdbc266ba76572e 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 40c8ef935104bab4ea2f8b2b5919450b55165f60 
>   3rdparty/libprocess/src/openssl.cpp 
> bd05866950e1043d9585a7c5fdc7b2147a233fd3 
>   3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71660/diff/3/
> 
> 
> Testing
> ---
> 
> cmake .. -ENABLE_SSL=1   # No Libevent here!
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71664: SSL Wrapper: Implemented BIO for SSL socket wrapper.

[Benno Evers]

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


Fix it, then Ship it!





3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 44 (patched)


We can replace the 57 by `BIO_get_new_index()` to avoid the conflict with 
libevent. (Even if we can't for some reason, we should at least choose 58 here 
to avoid conflict.)



3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 83 (patched)


It seems like `synchronized()` for atomic flags is implemented as a busy 
wait, was this choice intentional?



3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 258 (patched)


Should this be `get_libprocess_BIO_METHOD()` for consistency?


- Benno Evers


On Oct. 31, 2019, 1:29 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71664/
> ---
> 
> (Updated Oct. 31, 2019, 1:29 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This implements the OpenSSL basic I/O abstraction based on the
> libprocess event loop.  This BIO wraps a socket and handles the
> reading/writing, using io::read and io::write.
> 
> This BIO can be passed into an SSL context to enable usage of
> SSL translation functions like SSL_read and SSL_write.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71664/diff/2/
> 
> 
> Testing
> ---
> 
> cmake --build . --target process
> 
> A tiny bit of testing next patch.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71769: Gracefully handled duplicated volumes from non-conforming CSI plugins.

[Benno Evers]

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


Ship it!




Ship It!

- Benno Evers


On Nov. 14, 2019, 6:14 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71769/
> ---
> 
> (Updated Nov. 14, 2019, 6:14 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9956
> https://issues.apache.org/jira/browse/MESOS-9956
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the SLRP uses a plugin that does not conform to the CSI spec and
> reports duplicated volumes, the duplicate would be removed.
> 
> This is a backport of `43b86da531a889b1c4b1d7ca6acb2eb924ea01e1`. We
> are not backporting test changes as the original patch relies on
> periodic plugin polling.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
> 
> 
> Diff: https://reviews.apache.org/r/71769/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check` results in only known failures
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>