Re: Review Request 72761: Added the CSI server to the Mesos agent.

[Qian Zhang]

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




src/slave/slave.cpp
Lines 1747 (patched)


As we discussed in https://reviews.apache.org/r/72759/#comment310644 , this 
should be `csiServer->start(info.id())` because we need to expose agent ID to 
managed CSI plugin. Ditto for the code in `Slave::reregistered`.

And `Slave::reregistered` can be called multiple times when agent is 
running, so I think we need this logic 
https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/daemon.cpp#L165:L176
 in `CSIServer::start()` as well rather than always generating auth token every 
time when `CSIServer::start()` is called. WDYT?


- Qian Zhang


On Aug. 18, 2020, 1:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72761/
> ---
> 
> (Updated Aug. 18, 2020, 1:13 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a CSI server to the Mesos agent in both
> the agent binary and in tests.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 895057027165609c792089cd336988b65a3b305d 
>   src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 
>   src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d 
>   src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 
>   src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d 
>   src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 
>   src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 
>   src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed 
>   src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 
>   src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 
> 
> 
> Diff: https://reviews.apache.org/r/72761/diff/3/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.

[Qian Zhang]

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

(Updated Aug. 19, 2020, 10:52 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Minor changes.


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


Repository: mesos


Description
---

Exposed Mesos agent ID to managed CSI plugins.


Diffs (updated)
-

  src/csi/service_manager.hpp 76a80fb793d2293e56dec221da23290df6a4acc6 
  src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 
  src/resource_provider/storage/provider.cpp 
0a8dc26e66db0242474bcbbd0b2ff9cec81c58f5 
  src/slave/csi_server.cpp b435d5db194ffad736838a5b98494944d3264045 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72761: Added the CSI server to the Mesos agent.

[Greg Mann]

> On Aug. 18, 2020, 2:39 a.m., Qian Zhang wrote:
> > src/slave/main.cpp
> > Lines 583 (patched)
> > 
> >
> > I think it should be `--ip_discovery_command` or `--ip` flag, right?
> > 
> > https://github.com/apache/mesos/blob/1.10.0/src/slave/main.cpp#L328:L338
> > 
> > Ditto for the code in `src/tests/cluster.cpp`.

I switched to just using `process::address().ip` unconditionally. This should 
match the behavior of the SLRP, where we initialize the volume/service managers 
with the IP from the address in the Mesos agent's libprocess UPID.


- Greg


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


On Aug. 18, 2020, 5:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72761/
> ---
> 
> (Updated Aug. 18, 2020, 5:13 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a CSI server to the Mesos agent in both
> the agent binary and in tests.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 895057027165609c792089cd336988b65a3b305d 
>   src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 
>   src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d 
>   src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 
>   src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d 
>   src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 
>   src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 
>   src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed 
>   src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 
>   src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 
> 
> 
> Diff: https://reviews.apache.org/r/72761/diff/3/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72761: Added the CSI server to the Mesos agent.

[Greg Mann]

> On Aug. 18, 2020, 6:31 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 1746-1749 (original), 1746-1751 (patched)
> > 
> >
> > So do you want to implement `CSIServer::start()` in the way that I 
> > mentioned in this comment 
> > (https://reviews.apache.org/r/72779/#comment310664 )? If yes, then I think 
> > `CSIServer::start()` do not need to return anything (just a void method), 
> > and here we do not need to handle its result.

As discussed offline, I'll leave `start()` as it is so that we can use it to 
handle any async failures produced by the secret generator.


- Greg


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


On Aug. 18, 2020, 5:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72761/
> ---
> 
> (Updated Aug. 18, 2020, 5:13 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a CSI server to the Mesos agent in both
> the agent binary and in tests.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 895057027165609c792089cd336988b65a3b305d 
>   src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 
>   src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d 
>   src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 
>   src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d 
>   src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 
>   src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 
>   src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed 
>   src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 
>   src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 
> 
> 
> Diff: https://reviews.apache.org/r/72761/diff/3/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72787: Added basic tests for regexp-based constraints.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72771, 72738, 72739, 72741, 72742, 72774, 72743, 72744, 
72745, 72775, 72776, 72782, 72783, 72784, 72785, 72786, 72787]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:16.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh 2>&1 | 
tee build_72787"]

Error:
..
DC_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/re2-2020-07-06 
-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-strong -fPIC -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c ../../../src/common/resources.cpp  
-fPIC -DPIC -o common/.libs/libmesos_no_3rdparty_la-resources.o
/bin/bash ../libtool  --tag=CXX   --mode=compile g++ -DPACKAGE_NAME=\"mesos\" 
-DPACKAGE_TARNAME=\"mesos\" -DPACKAGE_VERSION=\"1.11.0\" 
-DPACKAGE_STRING=\"mesos\ 1.11.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" 
-DPACKAGE=\"mesos\" -DVERSION=\"1.11.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 -DENABLE_NVML=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=\"/tmp/SRC/build/mesos-1.11.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/tmp/SRC/build/mesos-1.11.0/_inst/libexec/mes
 os\" -DPKGDATADIR=\"/tmp/SRC/build/mesos-1.11.0/_inst/share/mesos\" 
-DPKGMODULEDIR=\"/tmp/SRC/build/mesos-1.11.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/re2-2020-07-06 
-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-strong -fPIC -fPIE -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c -o common/libmesos_no_
 3rdparty_la-type_utils.lo `test -f 'common/type_utils.cpp' || echo 
'../../../src/'`common/type_utils.cpp
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.11.0\" "-DPACKAGE_STRING=\"mesos 1.11.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.11.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 -DENABLE_NVML=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=\"/tmp/SRC/build/mesos-1.11.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/tmp/SRC/build/mesos-1.11.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/tmp/SRC/bu
 ild/mesos-1.11.0/_inst/share/mesos\" 
-DPKGMODULEDIR=\"/tmp/SRC/build/mesos-1.11.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 

Re: Review Request 72743: Wired up creating `OfferConstraintsFilter` into the schdeuler API.

[Benjamin Mahler]

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


Ship it!





src/master/master.cpp
Lines 2671 (patched)


I see there's a ticket in the epic for this, nice :)



src/master/master.cpp
Lines 3260 (patched)


Hm.. consider using a field path?

'UpdateFramework.offer_constraints' are not valid:


- Benjamin Mahler


On Aug. 14, 2020, 4:57 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72743/
> ---
> 
> (Updated Aug. 14, 2020, 4:57 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls actually construct
> the offer constraints filter from the `offer_constraints` field and
> pass it into the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
> 
> 
> Diff: https://reviews.apache.org/r/72743/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72759]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:16.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh 2>&1 | 
tee build_72759"]

Error:
ubuntu-16.04: Pulling from mesos/mesos-build
18d680d61657: Pulling fs layer
0addb6fece63: Pulling fs layer
78e58219b215: Pulling fs layer
eb6959a66df2: Pulling fs layer
1105027a5560: Pulling fs layer
c36946130a38: Pulling fs layer
6a6a5e68faab: Pulling fs layer
eb6959a66df2: Waiting
80e07249924c: Pulling fs layer
1105027a5560: Waiting
c36946130a38: Waiting
c4c63e2501db: Pulling fs layer
668b207c2829: Pulling fs layer
80e07249924c: Waiting
ed76dddad568: Pulling fs layer
668b207c2829: Waiting
c4c63e2501db: Waiting
0addb6fece63: Verifying Checksum
0addb6fece63: Download complete
78e58219b215: Verifying Checksum
78e58219b215: Download complete
eb6959a66df2: Verifying Checksum
eb6959a66df2: Download complete
18d680d61657: Verifying Checksum
18d680d61657: Download complete
c36946130a38: Verifying Checksum
c36946130a38: Download complete
80e07249924c: Verifying Checksum
80e07249924c: Download complete
6a6a5e68faab: Verifying Checksum
6a6a5e68faab: Download complete
668b207c2829: Download complete
c4c63e2501db: Verifying Checksum
c4c63e2501db: Download complete
ed76dddad568: Verifying Checksum
18d680d61657: Pull complete
0addb6fece63: Pull complete
78e58219b215: Pull complete
eb6959a66df2: Pull complete
1105027a5560: Verifying Checksum
1105027a5560: Download complete
1105027a5560: Pull complete
c36946130a38: Pull complete
6a6a5e68faab: Pull complete
80e07249924c: Pull complete
c4c63e2501db: Pull complete
668b207c2829: Pull complete
ed76dddad568: Pull complete
Digest: sha256:fa967cbcfb44f55708a3cbc87f245c6d29dd891464db558af56a03ee321526bb
Status: Downloaded newer image for mesos/mesos-build:ubuntu-16.04
docker.io/mesos/mesos-build:ubuntu-16.04
Cloning into '/tmp/SRC'...
Note: checking out '29d5b8121a6e1e91cda5fb2a4566a7c6d7b43c93'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b 

autoreconf: Entering directory `.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --warnings=all -I m4
autoreconf: configure.ac: tracing
configure.ac:1873: warning: cannot check for file existence when cross compiling
../../lib/autoconf/general.m4:2788: AC_CHECK_FILE is expanded from...
configure.ac:1873: the top level
configure.ac:1878: warning: cannot check for file existence when cross compiling
../../lib/autoconf/general.m4:2788: AC_CHECK_FILE is expanded from...
configure.ac:1878: the top level
configure.ac:2399: warning: AC_RUN_IFELSE called without default to allow cross 
compiling
../../lib/autoconf/general.m4:2759: AC_RUN_IFELSE is expanded from...
configure.ac:2399: the top level
autoreconf: running: libtoolize --copy
libtoolize: putting auxiliary files in '.'.
libtoolize: copying file './ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'm4'.
libtoolize: copying file 'm4/libtool.m4'
libtoolize: copying file 'm4/ltoptions.m4'
libtoolize: copying file 'm4/ltsugar.m4'
libtoolize: copying file 'm4/ltversion.m4'
libtoolize: copying file 'm4/lt~obsolete.m4'
autoreconf: running: /usr/bin/autoconf --warnings=all
configure.ac:1873: warning: cannot check for file existence when cross compiling
../../lib/autoconf/general.m4:2788: AC_CHECK_FILE is expanded from...
configure.ac:1873: the top level
configure.ac:1878: warning: cannot check for file existence when cross compiling
../../lib/autoconf/general.m4:2788: AC_CHECK_FILE is expanded from...
configure.ac:1878: the top level
configure.ac:2399: warning: AC_RUN_IFELSE called without default to allow cross 
compiling
../../lib/autoconf/general.m4:2759: AC_RUN_IFELSE is expanded from...
configure.ac:2399: the top level
autoreconf: configure.ac: not using Autoheader
autoreconf: running: automake --add-missing --copy --no-force --warnings=all
configure.ac:50: installing './ar-lib'
configure.ac:34: installing './compile'
configure.ac:24: installing './config.guess'
configure.ac:24: installing './config.sub'
configure.ac:46: installing './install-sh'
configure.ac:46: installing './missing'
3rdparty/Makefile.am:307: warning: source file '$(HTTP_PARSER)/http_parser.c' 
is in a subdirectory,
3rdparty/Makefile.am:307: but option 'subdir-objects' is disabled

Re: Review Request 72783: Added RE2 to the automake build.

[Andrei Sekretenko]

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

(Updated Aug. 18, 2020, 7:01 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added passing CXX, CXXFLAGS and unconditional `-fPIC`.


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


Repository: mesos


Description
---

This is a prerequisite to implementing regex-based offer constaints.


Diffs (updated)
-

  3rdparty/Makefile.am c277627ced18583868a3e50ed87f4a0ad69cddfb 
  3rdparty/versions.am 6f6195d8c616695f2077c6c36d96071360b1abb7 
  configure.ac a049945bd1611e0a23a9e327dd8df3b8dbd902d4 
  src/Makefile.am 6d68ed050f99889c142d49bbc72a9292ef64c836 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72774: Wired up the `OfferConstraintsFilter` into the hierarchical allocator.

[Benjamin Mahler]

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


Ship it!





src/master/allocator/mesos/hierarchical.cpp
Line 729 (original), 731-732 (patched)


To make this a bit clearer, maybe start with saying that we don't use 
std::insert here because XXX?



src/master/allocator/mesos/hierarchical.cpp
Lines 2164 (patched)


Period at the end :P



src/master/allocator/mesos/hierarchical.cpp
Lines 2400 (patched)


Period at the end :P


- Benjamin Mahler


On Aug. 14, 2020, 4:56 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72774/
> ---
> 
> (Updated Aug. 14, 2020, 4:56 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wired up the `OfferConstraintsFilter` into the hierarchical allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> e444e470eb085cea167f84f8540d1769d662c222 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5079942263132d09c6bd9abbdc8858cd2ef138 
> 
> 
> Diff: https://reviews.apache.org/r/72774/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72742: Added basic tests for the `OfferConstraintsFilter`.

[Andrei Sekretenko]

> On Aug. 18, 2020, 6:36 p.m., Benjamin Mahler wrote:
> > Great to have unit tests for this! Mainly left a suggestion about using a 
> > JSON approach to build the protobuf.
> > 
> > Do we want to add another test here for attributes with multiple entries? 
> > (i.e. showing that we only look at the first entry)

The test for multiple attributes is in https://reviews.apache.org/r/72776, 
after the equality constraints are implemented; I don't see how to add such a 
test without a constraint on the value.
JSON looks like a good idea, will try that.


- Andrei


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


On Aug. 14, 2020, 4:59 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72742/
> ---
> 
> (Updated Aug. 14, 2020, 4:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added basic tests for the `OfferConstraintsFilter`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e 
>   src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72742/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72742: Added basic tests for the `OfferConstraintsFilter`.

[Benjamin Mahler]

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



Great to have unit tests for this! Mainly left a suggestion about using a JSON 
approach to build the protobuf.

Do we want to add another test here for attributes with multiple entries? (i.e. 
showing that we only look at the first entry)


src/tests/master/offer_constraints_filter_tests.cpp
Lines 72-76 (patched)


Hm.. see my other comment below, might be able to avoid the 
`singleAttributeConstraintFilter` by using the same JSON approach to set up the 
protobuf in all the tests? It would also be nice to have all the 
OfferConstraintsFilter construction consistent (rather than some going through 
a helper and some not).



src/tests/master/offer_constraints_filter_tests.cpp
Lines 196-211 (patched)


For this and the others above, it's often pretty hard for a reader to parse 
through what the protobuf is with setting it up in C++, is it possible to just 
do a json conversion here as we've done in some other places?

```
Try json = JSON::parse(
R"~(
{
  "role_constraints": {
"role1": {
  "attribute_constraints": [{
"selector": {"attribute_name": "foo"},
"predicate": {"exists": {}}
  }]
},
"role2": {
  "attribute_constraints": [{
"selector": {"attribute_name": "bar"},
"predicate": {"not_exists": {}}
  }]
}
  }
})~");

ASSERT_SOME(json);

Try constraints = protobuf::parse(*json);

ASSERT_SOME(constraints);
```

This makes it really easy on the reader to see what the constraints look 
like.


- Benjamin Mahler


On Aug. 14, 2020, 4:59 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72742/
> ---
> 
> (Updated Aug. 14, 2020, 4:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added basic tests for the `OfferConstraintsFilter`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e 
>   src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72742/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72787: Added basic tests for regexp-based constraints.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [72787, 72786, 72785, 72784, 72783, 72782, 72776, 72775, 
72745, 72744, 72743, 72774, 72742, 72741, 72739, 72738, 72771]

Error:
2020-08-18 18:26:49 URL:https://reviews.apache.org/r/72775/diff/raw/ 
[2551/2551] -> "72775.patch" [1]
error: patch failed: src/master/allocator/mesos/offer_constraints_filter.cpp:92
error: src/master/allocator/mesos/offer_constraints_filter.cpp: patch does not 
apply

- Mesos Reviewbot


On Aug. 17, 2020, 8:24 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72787/
> ---
> 
> (Updated Aug. 17, 2020, 8:24 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10173
> https://issues.apache.org/jira/browse/MESOS-10173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added basic tests for regexp-based constraints.
> 
> 
> Diffs
> -
> 
>   src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72787/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 43-56 (patched)
> > 
> >
> > We should check that they aren't both set?
> 
> Andrei Sekretenko wrote:
> `oneof` guarantees that only one is set, regardless of the contents of 
> the serialized message.
> 
> And, at the very least, the code generated by the bundled protoc clearly 
> guarantees that only one of `has_*()` will return `true`. 
> For example, a message like
> ```
> message Foo{} 
>   
> message Bar{} 
>   
>   
>   
> message Msg{  
>   
>   oneof oneofname {   
>   
> Foo foo = 1;  
>   
> Bar bar = 2;  
>   
>   }   
>   
> }
> ```
> results in the following generated code:
> 
> ```  
> class Msg{
>   enum OneofnameCase {
>   
> kFoo = 1, 
>   
> kBar = 2, 
>   
> ONEOFNAME_NOT_SET = 0,
>   
>   };
>   ...
> }
> 
> inline bool Msg::has_foo() const {
>   
>   return oneofname_case() == kFoo;
>   
> } 
> inline Msg::OneofnameCase Msg::oneofname_case() const {   
>   
>   return Msg::OneofnameCase(_oneof_case_[0]); 
>   
> }
> ...
> 
> ```   
> 
> This means that such a check would protect only against two things:
>  - a major and **obvious** bug in `protoc`; if the current implementation 
> has any oneof bugs, this check will not help catch them
>  - us suddenly removing `oneof`; in this case we have bigger problems 
> than this one
> 
> The worst thing about adding this check is that right after adding 
> existence/equality constraiunts it will turn into checking that only one of 
> the **six** fields is set.
> Had we planned to have only two fields forever, this check would have 
> added clarity; with six fields, I doubt it makes things clearer.
> 
> Probably I should add a comment to remind readers that this is `oneof`, 
> or, better, some local static assertion that those fields are part of oneof.
> 
> I have to say that it is rather unfortunate that oneof field 
> accessors/setters have the same names and signatures as those of normal 
> fields...
> 
> Andrei Sekretenko wrote:
> Oops, I was looking at another oneof (for the predicate, which will grow 
> to six members soon). 
> Added a comment for that one, but will add a CHECK here, as we aren't 
> going to have more than two members in the foreseeable future in this one.
> 
> Benjamin Mahler wrote:
> Ah thanks for explaining this! I totally forgot that it was a one of 
> since the code here doesn't reveal it.
> 
> As a general rule for using oneof in protobuf, couldn't we leverage the 
> enum to make it clear in our code that only 1 can be set?
> 
> ```
> switch (selector_case()) {
>   case PseudoattributeType:
> ...
> break;
>   case attributeName:
> ...
> break;
>   case ONEOF_NAME_NOT_SET:
> ...
> break;
> }
> ```

We can, and we should. 
The code around CSI volume-related oneofs already does that; not sure how I 
overlooked that `_case()` is, in fact, well-documented in the protobuf docs.

That said, there is only one `oneof` in Mesos for which this convention is not 
used: `ContainerFileOperation::parameters`.


> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 134-143 (patched)
> > 
> >
> > I think we allow multiple entries for the same attribute key, on the 
> > agent side? Not sure if anyone uses that.
> 
> Andrei Sekretenko wrote:
> Good point. 
> We allow, and this is valid. I don't see any code/document disallowing 
> that; moreover, IIRC some doc mentions this explicitly.
> 
> Unconditionally taking the first attribute with the specified 

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Lines 76 (patched)
> > 
> >
> > Hm.. not obvious to me why we need this, can you explain in a comment?

Changed to the alternative (`=default`ed constructors/destructor after 
`OfferConstraintsFilterImpl`), hopefully that one requres fewer comments.

Please take a look; maybe even that alternative should be explained more 
extensively than I do in this patch.


> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 83-93 (patched)
> > 
> >
> > Ditto here for using the enum version, then there's no need for the 
> > NOTE at all :)
> > 
> > ```
> >   static Try create(
> >   AttributeConstraint::Predicate&& predicate)
> >   {
> > using Self = AttributeConstraintPredicate;
> > 
> > switch (predicate.predicate_case()) {
> >   case kExists:return Self(Exists{});
> >   case kNotExists: return Self(NotExists{});
> >   case ONEOF_NAME_NOT_SET: return Error("Unknown predicate type");
> > }
> >   }
> > ```
> > 
> > (Not sure if compiler will see that there's always a return, so I guess 
> > the last one might need to break and return Error outside.

I'm not sure all compilers will see that. Added UNREACHABLE().


> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 99-113 (patched)
> > 
> >
> > I don't know if we're saving much here with the additional abstraction:
> > 
> > ```
> >   // The following helper structs can apply the predicate using
> >   // overloads for the three cases:
> >   //   (1) Nothing   -> non existent (pseudo) attribute
> >   //   (2) string-> pseudo attribute value
> >   //   (3) Attribute -> named attribute value
> > 
> >   struct Exists
> >   {
> > bool apply(const Nothing&) const { return false; }
> > bool apply(const string&) const { return true; }
> > bool apply(const Attribute&) const { return true; }
> >   };
> >   
> >   struct NotExists
> >   {
> > bool apply(const Nothing&) const { return true; }
> > bool apply(const string&) const { return false; }
> > bool apply(const Attribute&) const { return false; }
> >   };
> > ```
> > 
> > I find thinking about the correctness of `negated` not that trivial vs 
> > this direct version. Maybe for others we add later the negated template 
> > will be worth it, but for Exists/NotExists it doesn't seem worth it?

Removed this layer. 
Now that the text equality/match constraints are NOT exact negation (the 
counterparts in pairs behave identically for existing non-TEXT) and thus are 
not templated, this one doesn't help improve consistency.


- Andrei


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


On Aug. 18, 2020, 6:01 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72741/
> ---
> 
> (Updated Aug. 18, 2020, 6:01 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch implements an offer filtering object that supports the
> Exists/NotExists offer constraints, and adds it into the allocator
> interface.
> 
> More constraints will be added to this filter in further patches.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
>   src/CMakeLists.txt a976dc12328f42d2268b4b5d86a934bf0c754594 
>   src/Makefile.am 6d68ed050f99889c142d49bbc72a9292ef64c836 
>   src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
> 
> 
> Diff: https://reviews.apache.org/r/72741/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

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

(Updated Aug. 18, 2020, 6:01 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch implements an offer filtering object that supports the
Exists/NotExists offer constraints, and adds it into the allocator
interface.

More constraints will be added to this filter in further patches.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
  src/CMakeLists.txt a976dc12328f42d2268b4b5d86a934bf0c754594 
  src/Makefile.am 6d68ed050f99889c142d49bbc72a9292ef64c836 
  src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
  src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72738: Added protobuf messages for constraints-based offer filtering.

[Andrei Sekretenko]

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

(Updated Aug. 18, 2020, 5:59 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Multiple attributes: added a comment in `Selector` and a warining into the docs.


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


Repository: mesos


Description
---

This patch adds framework's offer constraints  into `Subscribe` and
`UpdateFramework` calls, which is a prerequisite to implementing
constraints-based offer filtering (see MESOS-10161).


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 
  include/mesos/scheduler/scheduler.proto 
6e1639a9baf017fa87b274daeedc821389216ddc 
  include/mesos/v1/scheduler/scheduler.proto 
eb5fdeb984b28403bd8281742bd0a5f2053863e3 
  src/internal/devolve.cpp 4527c522473b74622055e1765740e3706b95afdb 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72759: Improved CSI service manager to set node ID for managed CSI plugins.

[Qian Zhang]

> On Aug. 14, 2020, 6:21 a.m., Greg Mann wrote:
> > src/csi/service_manager.cpp
> > Lines 740 (patched)
> > 
> >
> > Where does this env var name come from, 'MESOS_NODE_ID'?
> 
> Qian Zhang wrote:
> Orginially I wanted to just name it `NODE_ID`, however I think it is too 
> generic and may be conflict with other env var used by the CSI plugin. So I 
> changed to add a `MESOS_` prefix just like other env vars that Mesos sets for 
> containers, e.g. `MESOS_FRAMEWORK_ID`, `MESOS_SLAVE_ID`, 
> `MESOS_AGENT_ENDPOINT`. Or maybe we should name it `MESOS_AGENT_IP`?
> 
> Greg Mann wrote:
> Hm, I think this may not yield a good ID for the agent, since I think 
> it's possible that the value of `self().address.ip` could be the same across 
> all nodes in the cluster - I think it could be `0.0.0.0`/`INADDR_ANY`, for 
> example. Do you think we need to inject the Mesos agent ID into the service 
> manager so that we can use it as the node ID here?

I think `self().address.ip` could not be `0.0.0.0/INADDR_ANY`, see 
https://github.com/apache/mesos/blob/1.10.0/3rdparty/libprocess/src/process.cpp#L1142:L1159
 for details.

> Do you think we need to inject the Mesos agent ID into the service manager so 
> that we can use it as the node ID here?

Yeah, that's my original thought, but it may involve a bit more code changes in 
some other components, like CSI server, SLRP. But after second thought I think 
it should be OK and makes more sense to use Mesos agent ID. I have updated this 
patch accordingly. And could you please update 
https://reviews.apache.org/r/72761 by passing Mesos agent ID to CSI server 
(e.g. as a parameter of `CSIServer::start()`).


- Qian


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


On Aug. 12, 2020, 7:47 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72759/
> ---
> 
> (Updated Aug. 12, 2020, 7:47 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10175
> https://issues.apache.org/jira/browse/MESOS-10175
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved CSI service manager to set node ID for managed CSI plugins.
> 
> 
> Diffs
> -
> 
>   src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 
> 
> 
> Diff: https://reviews.apache.org/r/72759/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Aug. 18, 2020, 4:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72759/
> ---
> 
> (Updated Aug. 18, 2020, 4:22 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10175
> https://issues.apache.org/jira/browse/MESOS-10175
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed Mesos agent ID to managed CSI plugins.
> 
> 
> Diffs
> -
> 
>   src/csi/service_manager.hpp 76a80fb793d2293e56dec221da23290df6a4acc6 
>   src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 
>   src/resource_provider/storage/provider.cpp 
> 0a8dc26e66db0242474bcbbd0b2ff9cec81c58f5 
>   src/slave/csi_server.cpp b435d5db194ffad736838a5b98494944d3264045 
> 
> 
> Diff: https://reviews.apache.org/r/72759/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.

[Qian Zhang]

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

(Updated Aug. 19, 2020, 12:22 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comment.


Summary (updated)
-

Exposed Mesos agent ID to managed CSI plugins.


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


Repository: mesos


Description (updated)
---

Exposed Mesos agent ID to managed CSI plugins.


Diffs (updated)
-

  src/csi/service_manager.hpp 76a80fb793d2293e56dec221da23290df6a4acc6 
  src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 
  src/resource_provider/storage/provider.cpp 
0a8dc26e66db0242474bcbbd0b2ff9cec81c58f5 
  src/slave/csi_server.cpp b435d5db194ffad736838a5b98494944d3264045 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.

[Greg Mann]

> On Aug. 13, 2020, 10:21 p.m., Greg Mann wrote:
> > src/csi/service_manager.cpp
> > Lines 740 (patched)
> > 
> >
> > Where does this env var name come from, 'MESOS_NODE_ID'?
> 
> Qian Zhang wrote:
> Orginially I wanted to just name it `NODE_ID`, however I think it is too 
> generic and may be conflict with other env var used by the CSI plugin. So I 
> changed to add a `MESOS_` prefix just like other env vars that Mesos sets for 
> containers, e.g. `MESOS_FRAMEWORK_ID`, `MESOS_SLAVE_ID`, 
> `MESOS_AGENT_ENDPOINT`. Or maybe we should name it `MESOS_AGENT_IP`?
> 
> Greg Mann wrote:
> Hm, I think this may not yield a good ID for the agent, since I think 
> it's possible that the value of `self().address.ip` could be the same across 
> all nodes in the cluster - I think it could be `0.0.0.0`/`INADDR_ANY`, for 
> example. Do you think we need to inject the Mesos agent ID into the service 
> manager so that we can use it as the node ID here?
> 
> Qian Zhang wrote:
> I think `self().address.ip` could not be `0.0.0.0/INADDR_ANY`, see 
> https://github.com/apache/mesos/blob/1.10.0/3rdparty/libprocess/src/process.cpp#L1142:L1159
>  for details.
> 
> > Do you think we need to inject the Mesos agent ID into the service 
> manager so that we can use it as the node ID here?
> 
> Yeah, that's my original thought, but it may involve a bit more code 
> changes in some other components, like CSI server, SLRP. But after second 
> thought I think it should be OK and makes more sense to use Mesos agent ID. I 
> have updated this patch accordingly. And could you please update 
> https://reviews.apache.org/r/72761 by passing Mesos agent ID to CSI server 
> (e.g. as a parameter of `CSIServer::start()`).

Yep, sgtm.


- Greg


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


On Aug. 18, 2020, 4:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72759/
> ---
> 
> (Updated Aug. 18, 2020, 4:22 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10175
> https://issues.apache.org/jira/browse/MESOS-10175
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed Mesos agent ID to managed CSI plugins.
> 
> 
> Diffs
> -
> 
>   src/csi/service_manager.hpp 76a80fb793d2293e56dec221da23290df6a4acc6 
>   src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 
>   src/resource_provider/storage/provider.cpp 
> 0a8dc26e66db0242474bcbbd0b2ff9cec81c58f5 
>   src/slave/csi_server.cpp b435d5db194ffad736838a5b98494944d3264045 
> 
> 
> Diff: https://reviews.apache.org/r/72759/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72779: Initialized plugins lazily in the CSI server.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [72779, 72728, 72761, 72727, 72726, 72754, 72753, 72734, 
72733, 72690, 72716, 72732]

Error:
2020-08-18 08:36:49 URL:https://reviews.apache.org/r/72779/diff/raw/ 
[9841/9841] -> "72779.patch" [1]
error: patch failed: src/slave/csi_server.cpp:236
error: src/slave/csi_server.cpp: patch does not apply

- Mesos Reviewbot


On Aug. 15, 2020, 11:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72779/
> ---
> 
> (Updated Aug. 15, 2020, 11:54 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the CSI server to lazily initialize CSI
> plugins on-demand when the server receives publish or
> unpublish calls for a plugin which is not already
> successfully initialized.
> 
> 
> Diffs
> -
> 
>   src/slave/csi_server.cpp a9a399579c6f7683b36e72582da20ea39311a222 
> 
> 
> Diff: https://reviews.apache.org/r/72779/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72754: Enabled the `volume/csi` isolator in `MesosContainerizer`.

[Qian Zhang]

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

(Updated Aug. 18, 2020, 2:59 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Enabled the `volume/csi` isolator in `MesosContainerizer`.


Diffs (updated)
-

  src/slave/containerizer/containerizer.hpp 
2b3c4c0363a6362c62695f087ea25248657d99df 
  src/slave/containerizer/containerizer.cpp 
9e44e5e0a41e48121f147778295a07b10b03bcf2 
  src/slave/containerizer/mesos/containerizer.hpp 
56e4c49733ac4e236b81ebf9e3238bf8a189c9f2 
  src/slave/containerizer/mesos/containerizer.cpp 
3c1840cae4befe09581ea4efae855d552bd19b05 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72779: Initialized plugins lazily in the CSI server.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [72779, 72728, 72761, 72727, 72726, 72754, 72753, 72734, 
72733, 72690, 72716, 72732]

Error:
2020-08-18 06:44:21 URL:https://reviews.apache.org/r/72779/diff/raw/ 
[9841/9841] -> "72779.patch" [1]
error: patch failed: src/slave/csi_server.cpp:236
error: src/slave/csi_server.cpp: patch does not apply

- Mesos Reviewbot


On Aug. 15, 2020, 11:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72779/
> ---
> 
> (Updated Aug. 15, 2020, 11:54 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the CSI server to lazily initialize CSI
> plugins on-demand when the server receives publish or
> unpublish calls for a plugin which is not already
> successfully initialized.
> 
> 
> Diffs
> -
> 
>   src/slave/csi_server.cpp a9a399579c6f7683b36e72582da20ea39311a222 
> 
> 
> Diff: https://reviews.apache.org/r/72779/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72761: Added the CSI server to the Mesos agent.

[Qian Zhang]

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




src/slave/slave.cpp
Lines 1746-1749 (original), 1746-1751 (patched)


So do you want to implement `CSIServer::start()` in the way that I 
mentioned in this comment (https://reviews.apache.org/r/72779/#comment310664 )? 
If yes, then I think `CSIServer::start()` do not need to return anything (just 
a void method), and here we do not need to handle its result.


- Qian Zhang


On Aug. 18, 2020, 1:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72761/
> ---
> 
> (Updated Aug. 18, 2020, 1:13 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a CSI server to the Mesos agent in both
> the agent binary and in tests.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 895057027165609c792089cd336988b65a3b305d 
>   src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 
>   src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d 
>   src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 
>   src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d 
>   src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 
>   src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 
>   src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed 
>   src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 
>   src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 
> 
> 
> Diff: https://reviews.apache.org/r/72761/diff/3/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>