Re: Review Request 72707: Added interface for the CSI server.

[Greg Mann]

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

(Updated July 31, 2020, 6:53 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 interface for the CSI server. This component will hold
objects associated with CSI plugins running on the agent.


Diffs (updated)
-

  src/slave/csi_server.hpp PRE-CREATION 


Diff: https://reviews.apache.org/r/72707/diff/6/

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


Testing
---

Details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 72707: Added interface for the CSI server.

[Greg Mann]

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

(Updated July 31, 2020, 6:48 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 interface for the CSI server. This component will hold
objects associated with CSI plugins running on the agent.


Diffs (updated)
-

  src/slave/csi_server.hpp PRE-CREATION 


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

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


Testing
---

Details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 72707: Added interface for the CSI server.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/csi_server.hpp
Lines 21 (patched)


Why do we need this field?


- Qian Zhang


On 七月 29, 2020, 5:37 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72707/
> ---
> 
> (Updated 七月 29, 2020, 5:37 a.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 interface for the CSI server. This component will hold
> objects associated with CSI plugins running on the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/csi_server.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72707/diff/4/
> 
> 
> Testing
> ---
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72707: Added interface for the CSI server.

[Greg Mann]

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

(Updated July 28, 2020, 9:37 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 (updated)
---

Added interface for the CSI server. This component will hold
objects associated with CSI plugins running on the agent.


Diffs (updated)
-

  src/slave/csi_server.hpp PRE-CREATION 


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

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


Testing (updated)
---

Details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 72707: Added interface for the CSI server.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/csi_server.hpp
Line 42 (original), 48 (patched)


I think we do not need `slave::` here. And it's better to name this 
parameter `_flags` to be consistent withe the other two parameters.



src/slave/csi_server.hpp
Line 59 (original), 66-67 (patched)


I think we need to return `std::string` by `publishVolume` rather than 
`unpublishVolume`.

And do we need the whole `CSIVolume` as the parameter? I think we only need 
plugin name and volume ID, right?


- Qian Zhang


On July 27, 2020, 1:37 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72707/
> ---
> 
> (Updated July 27, 2020, 1:37 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 interface for the CSI server.
> 
> 
> Diffs
> -
> 
>   src/slave/csi_server.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72707/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72707: Added interface for the CSI server.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72660, 72661, 72672, 72707]

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

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
80e07249924c: Pulling fs layer
c4c63e2501db: Pulling fs layer
668b207c2829: Pulling fs layer
ed76dddad568: Pulling fs layer
eb6959a66df2: Waiting
1105027a5560: Waiting
6a6a5e68faab: Waiting
c36946130a38: Waiting
ed76dddad568: Waiting
c4c63e2501db: Waiting
668b207c2829: Waiting
80e07249924c: Waiting
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
6a6a5e68faab: Verifying Checksum
6a6a5e68faab: Download complete
80e07249924c: Verifying Checksum
80e07249924c: Download complete
c4c63e2501db: Verifying Checksum
c4c63e2501db: Download complete
668b207c2829: Download complete
ed76dddad568: Download complete
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 '4e9708186b8a05e0e39f0d58c8aa370cff93292c'.

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 

Re: Review Request 72707: Added interface for the CSI server.

[Greg Mann]

> On July 24, 2020, 8:50 a.m., Qian Zhang wrote:
> > src/CMakeLists.txt
> > Lines 165 (patched)
> > 
> >
> > I think we should put this line to L153 under 
> > `slave/container_logger.cpp`.

Agreed. I just removed everything from this patch but the header for now; I 
have a patch in progress with the full implementation, will post soon.


- Greg


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


On July 27, 2020, 5:37 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72707/
> ---
> 
> (Updated July 27, 2020, 5:37 a.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 interface for the CSI server.
> 
> 
> Diffs
> -
> 
>   src/slave/csi_server.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72707/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72707: Added interface for the CSI server.

[Greg Mann]

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

(Updated July 27, 2020, 5:37 a.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 interface for the CSI server.


Diffs (updated)
-

  src/slave/csi_server.hpp PRE-CREATION 


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

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 72707: Added interface for the CSI server.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72660, 72707]

Passed command: 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

- Mesos Reviewbot


On July 24, 2020, 6 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72707/
> ---
> 
> (Updated July 24, 2020, 6 a.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 interface for the CSI server.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bcb5128f0e61af0d541502e4ed833da0487b7792 
>   src/Makefile.am a89919dd7d5ccbc4c5fa79d9a83616608f84ef49 
>   src/slave/csi_server.hpp PRE-CREATION 
>   src/slave/csi_server.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72707/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72707: Added interface for the CSI server.

[Qian Zhang]

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




src/CMakeLists.txt
Lines 165 (patched)


I think we should put this line to L153 under `slave/container_logger.cpp`.



src/Makefile.am
Lines 1326-1327 (patched)


Ditto.



src/slave/csi_server.hpp
Lines 41-43 (patched)


Instead of `configDir`, I think we need agent `Flags` and in this `create` 
method we need to scan the directory `--csi_plugin_config_dir` to load the 
supported CSI plugin configs.

And I think we also need secret generator to generate auth token for 
service manager to use, see:

https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/daemon.cpp#L543

https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/storage/provider.cpp#L575



src/slave/csi_server.hpp
Lines 52 (patched)


I think we may need this method to return the target path at which the 
volume is published, and then in `volume/csi` we can bind mount that path into 
the container's mount namespace for container to use.


- Qian Zhang


On July 24, 2020, 2 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72707/
> ---
> 
> (Updated July 24, 2020, 2 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 interface for the CSI server.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bcb5128f0e61af0d541502e4ed833da0487b7792 
>   src/Makefile.am a89919dd7d5ccbc4c5fa79d9a83616608f84ef49 
>   src/slave/csi_server.hpp PRE-CREATION 
>   src/slave/csi_server.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72707/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72707: Added interface for the CSI server.

[Greg Mann]

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

(Updated July 24, 2020, 6 a.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 interface for the CSI server.


Diffs (updated)
-

  src/CMakeLists.txt bcb5128f0e61af0d541502e4ed833da0487b7792 
  src/Makefile.am a89919dd7d5ccbc4c5fa79d9a83616608f84ef49 
  src/slave/csi_server.hpp PRE-CREATION 
  src/slave/csi_server.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Greg Mann