mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 70213: Added the `VolumeManager` interface to manage CSI volumes.
Date Mon, 25 Mar 2019 10:03:39 GMT


> On March 19, 2019, 3: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?

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.


> On March 19, 2019, 3:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.hpp
> > Lines 77 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132465#file2132465line77>
> >
> >     Shouldn't this be version-agnostic?
> 
> Chun-Hung Hsiao wrote:
>     Addressed in r/70247 and r/70248. These patches are put later in chain because I'm
considering if the first few refactoring patches should be backported for backporting bug
fixes in the future. But as you suggested, this is too big of a change so could be a terrible
idea. Instead maybe we should just do extra backporting efforts in the future, and only for
critical bugs.
>     
>     If this is the plan, I could reorder the patches.
>     
>     Dropping the following "ditto" issues but leave this one open for discussion.

What about only backporting v0 impls, but still introducing version-agnostic interfaces? Introducing
such an inconsistent interface seems confusing to me, especially if we only backport an intermediate
state.


> On March 19, 2019, 3:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.hpp
> > Lines 115-117 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132465#file2132465line115>
> >
> >     Am I reading correctly that the return boolean is not dynamic and just maps
onto whether the controller has `CREATE_DELETE_VOLUME` or not?
> 
> Chun-Hung Hsiao wrote:
>     Yes you're right lol. This roughly matches the semantics defined here: https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L2102
>     
>     Again, not sure if we should expose the CSI capabilities instead. WDYT?

It would be nice to see how this would be used. Do you want to distinguish transient from
permanent failures, or would a `Future<Nothing>` work just as well?


> On March 19, 2019, 3: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.

Let's add the needed parameters to the factory now.


> On March 19, 2019, 3:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.cpp
> > Lines 44 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132466#file2132466line44>
> >
> >     `#include <stout/errorbase.hpp>`
> 
> Chun-Hung Hsiao wrote:
>     I intentionally don't want to use `errorbase.hpp` as `error.hpp` seems to be a more
proper public header for inclusion. Dropping. Feel free to reopen.

We do that elsewhere as well, sounds okay.


- Benjamin


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


On March 15, 2019, 6: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, 6: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