mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 70213: Added the `VolumeManager` interface to manage CSI volumes.
Date Tue, 26 Mar 2019 05:05:18 GMT


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.hpp
> > Lines 74-75 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132465#file2132465line74>
> >
> >     Do we have a chance here to translate from CSI semantics to our own semantics
instead of leaking the CSI protocol? I think not using a magic value of `0` would be nice.
> >     
> >     Seems like we could either return a `Failure` if `GET_CAPACITY` is not supported
or some `Option<Bytes>::none()`. This would likely require capability knowledge.
> 
> Chun-Hung Hsiao wrote:
>     I'm actually debating if we should expose CSI capabilities. Given that the capabilities
might change, do you think it's a good idea to expose that?
> 
> Benjamin Bannier wrote:
>     Wouldn't they'd only be exposed in the documentation? We could replace that part
with _can permanently not be determined_ so we can use failed futures for retryable errors
and ready nones for the unsupported providers.

The caller, i.e., SLRP, won't know why it fails. If the plugin doesn't support `GET_CAPACITY`,
SLRP should still work with handling pre-existing volumes. So we either needs to expose the
capability to SLRP, or don't fail here.


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.cpp
> > Lines 28-35 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132466#file2132466line28>
> >
> >     How do you plan to dispatch here once we need to support both `v0` and `v1`?
Can we infer what needs to be done from these arguments alone?
> 
> Chun-Hung Hsiao wrote:
>     Nope. This will return `Future<Owned<VolumeManager>>` in the future,
where a `ServiceManager` is created first to detect CSI versions then the proper `VolumeManager`
is created accordingly.
> 
> Benjamin Bannier wrote:
>     Let's add the needed parameters to the factory now.

I mean we cannot infer the needed version just from the params, but can do detection so no
extra param is needed.


- Chun-Hung


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


On March 15, 2019, 5:14 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70213/
> -----------------------------------------------------------
> 
> (Updated March 15, 2019, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9622
>     https://issues.apache.org/jira/browse/MESOS-9622
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `VolumeManager` is a wrapper for SLRP to use v0 and v1 CSI plugins
> polymorphically. It will be managing volume lifecycles and checkpoints
> and making the actual CSI calls for SLRP and SERP in the future.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/csi/volume_manager.hpp PRE-CREATION 
>   src/csi/volume_manager.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70213/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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