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

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72660, 72661, 72672, 72681, 72715, 72707, 72716, 72690, 
72733, 72734, 72753, 72754]

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_72754"]

Error:
..
bs/\" -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/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/in
 clude  -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-strong -fPIC -fPIE -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c -o 
scheduler/libmesos_no_3rdparty_la-scheduler.lo `test -f 
'scheduler/scheduler.cpp' || echo '../../../src/'`scheduler/scheduler.cpp
/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/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 
secret/libmesos_no_3rdparty_la-resolver.lo `test
  -f 'secret/resolver.cpp' || echo '../../../src/'`secret/resolver.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 

Re: Review Request 72716: Added implementation of the CSI server.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Aug. 7, 2020, 3 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> ---
> 
> (Updated Aug. 7, 2020, 3 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of the CSI server.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c 
>   src/slave/csi_server.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72716/diff/5/
> 
> 
> Testing
> ---
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72716: Added implementation of the CSI server.

[Qian Zhang]

> On Aug. 7, 2020, 8:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 200 (patched)
> > 
> >
> > Do we need to include `info.type()` in the container prefix? Otherwise 
> > the container prefix for all the managed CSI plugins will be same which may 
> > not be good for debugging.
> 
> Greg Mann wrote:
> The plugin name and type already get added into the container ID after 
> the prefix, so I think the current code is sufficient: 
> https://github.com/apache/mesos/blob/c78dc333fc893a43d40dc33299a61987198a6ea9/src/csi/service_manager.cpp#L117-L121
> 
> WDYT?

Yeah, you are right!


> On Aug. 7, 2020, 8:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 336 (patched)
> > 
> >
> > Do we need to check if `volume/csi` isolator is enabled? Like:
> > ```
> > if (!strings::contains(flags.isolation, "volume/csi")) {
> >   return Error("...");
> > }
> > ```
> > 
> > I think to make the whole CSI volume feature work, both `CSIServer` and 
> > `volume/csi` isolator need to be enabled.
> > 
> > And in which condition will we call `CSIServer::create` to create 
> > `CSISever`? When `--csi_plugin_config_dir` is specified? If so, I think 
> > here we need a `CHECK` rather than an `if`.
> 
> Greg Mann wrote:
> I agree that we should check for the CSI isolator. However, I don't think 
> we should use a CHECK within this `create()` method. The `Try` return type 
> provides the perfect mechanism for surfacing any failures, which we can 
> handle in the agent.

Yeah, I agree. So could you please add the check for the CSI isolator here?

And I guess we will call `CSIServer::create` to create CSI sever **only** when 
`--csi_plugin_config_dir` is specified, right?


> On Aug. 7, 2020, 8:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 413-415 (patched)
> > 
> >
> > Do we need to define `started` as a promise here? Can we just define it 
> > as a future?
> > ```
> > started = process::dispatch(process.get(), ::start);
> > return started;
> > ```
> 
> Greg Mann wrote:
> Yep, we can't use a simple Future because we don't initiate startup 
> during server construction, as is the case in the volume manager. 
> `started.associate()` allows us to initiate startup using the existing Future 
> within the Promise. If we use a raw Future, then the only way to initiate 
> startup is to overwrite the stored Future, onto which some publish/unpublish 
> calls may have already been chained.

I see, thank you!


- Qian


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


On Aug. 7, 2020, 3 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> ---
> 
> (Updated Aug. 7, 2020, 3 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of the CSI server.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c 
>   src/slave/csi_server.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72716/diff/5/
> 
> 
> Testing
> ---
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72733: Implemented the `prepare` method of `volume/csi` isolator.

[Qian Zhang]

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

(Updated Aug. 10, 2020, 10:37 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Implemented the `prepare` method of `volume/csi` isolator.


Diffs (updated)
-

  include/mesos/mesos.proto 0f91d88abba4bd49b46676e956211f1cf4e7219b 
  include/mesos/v1/mesos.proto f25db8aeefd721653b2027282abbae8f2e6a40f3 
  src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
  src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
  src/common/validation.cpp 14a8c7b9d70c303b527e5e43012999471ced2d78 
  src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/csi/state.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/csi/state.proto PRE-CREATION 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72716: Added implementation of the CSI server.

[Qian Zhang]

> On July 29, 2020, 11:27 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 233-235 (patched)
> > 
> >
> > I am just curious what would happen if any of the initialization logic 
> > fail, how will the failure be propogated back?
> 
> Greg Mann wrote:
> I updated the server so that now `start()` returns a future associated 
> with the initialization.
> 
> Qian Zhang wrote:
> I see. And I guess `CSIServer::start()` will be called in 
> `Slave::registered` and `Slave::reregistered`, right? I am just wondering how 
> we are going to handle the returned future there. Are we going to register an 
> `onAny` callback and log an error message if it is a failed future?
> 
> Greg Mann wrote:
> Yea I think we have to decide how to handle failures of CSI server 
> initialization. I might propose a timeout in the agent, after which we log an 
> error? And we could provide a task status message perhaps when task launches 
> fail because the CSI server failed to initialize?
> 
> In any case, I think the interface offered by the current patch set will 
> be sufficient to let us handle the failed initialization case, WDYT?
> 
> Qian Zhang wrote:
> I took a look at the code of local resource provider daemon and I found 
> it just log an error message in its `start` method:
> 
> https://github.com/apache/mesos/blob/1.10.0/src/slave/slave.cpp#L1740:L1742
> 
> https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/daemon.cpp#L188:L191
> 
> Do you think if we can do the similar?
> 
> Greg Mann wrote:
> I was worried about maintaining ordering of calls made before startup, 
> but as we discussed offline, I think I was being too paranoid :)
> 
> I'm switching the patch to the kind of approach used in the daemon.
> 
> Greg Mann wrote:
> lol sorry I replied on the wrong issue :)
> 
> I think we can log an error message as well, but it does also seem 
> appropriate to me to return a future associated with the startup. We can 
> decide what to do with that future in the agent, but might as well return it?
> 
> Greg Mann wrote:
> I decided not to log an error in the server, I think the proper place to 
> log would be at the callsite where we handle the future returned by 
> `start()`, WDYT?
> 
> Qian Zhang wrote:
> So do you mean we should log an error in `Slave::registered` and 
> `Slave::reregistered`?
> 
> Greg Mann wrote:
> It depends on how exactly we initialize the CSI server in the agent; I'm 
> not sure that it would necessarily be in those functions. In any case, since 
> `start()` returns a Future I don't think we need to log anything here in the 
> server, unless we think there are multiple classes of failures, some of which 
> we would want to crash for and some of which we would want to just log?

IMHO we need to call `CSIServer::start` in `Slave::registered` and 
`Slave::reregistered` (i.e. right after the agent's state is changed to 
`RUNNING`), do we have any other options?


- Qian


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


On Aug. 7, 2020, 3 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> ---
> 
> (Updated Aug. 7, 2020, 3 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of the CSI server.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c 
>   src/slave/csi_server.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72716/diff/5/
> 
> 
> Testing
> ---
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [72754, 72753, 72734, 72733, 72690, 72716, 72707, 72715, 
72681, 72672, 72661, 72660]

Error:
2020-08-10 02:20:36 URL:https://reviews.apache.org/r/72733/diff/raw/ 
[17892/17892] -> "72733.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp:16
error: src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp: patch 
does not apply

- Mesos Reviewbot


On Aug. 10, 2020, 9:45 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72754/
> ---
> 
> (Updated Aug. 10, 2020, 9:45 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10156
> https://issues.apache.org/jira/browse/MESOS-10156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled the `volume/csi` isolator in `MesosContainerizer`.
> 
> 
> Diffs
> -
> 
>   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/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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/
---

Review request for mesos, Andrei Budnik and Greg Mann.


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


Repository: mesos


Description
---

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


Diffs
-

  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/1/


Testing
---


Thanks,

Qian Zhang

Review Request 72753: Implemented the `recover` method of `volume/csi` isolator.

[Qian Zhang]

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

Review request for mesos, Andrei Budnik and Greg Mann.


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


Repository: mesos


Description
---

Implemented the `recover` method of `volume/csi` isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION 


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


Testing
---


Thanks,

Qian Zhang