mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 72716: Added implementation of the CSI server.
Date Fri, 07 Aug 2020 15:13:58 GMT


> On Aug. 7, 2020, 12:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 200 (patched)
> > <https://reviews.apache.org/r/72716/diff/5/?file=2237439#file2237439line200>
> >
> >     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.

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?


> On Aug. 7, 2020, 12:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 336 (patched)
> > <https://reviews.apache.org/r/72716/diff/5/?file=2237439#file2237439line336>
> >
> >     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`.

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.


> On Aug. 7, 2020, 12:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 413-415 (patched)
> > <https://reviews.apache.org/r/72716/diff/5/?file=2237439#file2237439line413>
> >
> >     Do we need to define `started` as a promise here? Can we just define it as a
future?
> >     ```
> >     started = process::dispatch(process.get(), &CSIServerProcess::start);
> >     return started;
> >     ```

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.


- Greg


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


On Aug. 7, 2020, 7 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2020, 7 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 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message