You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2019/03/27 02:08:34 UTC
Review Request 70314: Supported destroying preprovisioned CSI volumes
in SLRP.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70314/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Jie Yu.
Bugs: MESOS-9540
https://issues.apache.org/jira/browse/MESOS-9540
Repository: mesos
Description
-------
SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
capability, this operation will be a no-op; otherwise the underlying CSI
volume will be deprovisioned.
Diffs
-----
src/resource_provider/storage/provider.cpp fea623c292158deb1b4b4b9ab1ac208031471519
Diff: https://reviews.apache.org/r/70314/diff/1/
Testing
-------
make check
The new code path is tested later in this chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 70314: Supported destroying preprovisioned CSI
volumes in SLRP.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line1>
> >
> > This patch should contain a test of introduced behavior.
Test done later in chain. I prefer splitting the fix that is targeting for backporting and the test, so the test needs not to be backported. Or do you think we should backport the test as well?
> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2702 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line2734>
> >
> > All this local checking of capabilities at call sites instead of in `call` is getting cumbersome and error-prone. Would be great to clean that up eventually by moving it to a single place.
`call` should be a darn simple wrapper. The call site has more context about what to check and how to react. In this example, the lack of this capability shouldn't result in an error.
> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3202-3203 (original), 3194-3196 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line3227>
> >
> > Let's use a `switch` now that we have three cases here.
Good point. These checks actually don't give us much value and I was thinking about removing them. Using a `switch` and moving it into the continuation instead, could enable the compiler to check if we forget to clear some field in the future.
> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3237-3239 (original), 3235-3237 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line3269>
> >
> > No quotes around `resource`; let's also fix the wrapping so that related entities are on the same line (`"resource provider"` and `info.id()`; `"resource"` and `resource`; opening and closing quotes).
> > ```
> > LOG(INFO)
> > << "Reconciling storage pools for resource provider " << info.id()
> > << " after resource '" << resource << "' has been freed";
We usually quote resoucre in the codebase.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70314/#review214091
-----------------------------------------------------------
On March 27, 2019, 3:45 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70314/
> -----------------------------------------------------------
>
> (Updated March 27, 2019, 3:45 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
> If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
> capability, this operation will be a no-op; otherwise the underlying CSI
> volume will be deprovisioned.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
>
>
> Diff: https://reviews.apache.org/r/70314/diff/2/
>
>
> Testing
> -------
>
> make check
>
> The new code path is tested later in this chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70314: Supported destroying preprovisioned CSI
volumes in SLRP.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70314/#review214091
-----------------------------------------------------------
Fix it, then Ship it!
src/resource_provider/storage/provider.cpp
Line 1 (original), 1 (patched)
<https://reviews.apache.org/r/70314/#comment300260>
This patch should contain a test of introduced behavior.
src/resource_provider/storage/provider.cpp
Lines 2646 (patched)
<https://reviews.apache.org/r/70314/#comment300256>
Add a comment here documenting what this branch models.
src/resource_provider/storage/provider.cpp
Lines 2702 (patched)
<https://reviews.apache.org/r/70314/#comment300258>
All this local checking of capabilities at call sites instead of in `call` is getting cumbersome and error-prone. Would be great to clean that up eventually by moving it to a single place.
src/resource_provider/storage/provider.cpp
Lines 2725 (patched)
<https://reviews.apache.org/r/70314/#comment300257>
Great, should have been here all along.
src/resource_provider/storage/provider.cpp
Lines 3202-3203 (original), 3194-3196 (patched)
<https://reviews.apache.org/r/70314/#comment300259>
Let's use a `switch` now that we have three cases here.
src/resource_provider/storage/provider.cpp
Lines 3237-3239 (original), 3235-3237 (patched)
<https://reviews.apache.org/r/70314/#comment300255>
No quotes around `resource`; let's also fix the wrapping so that related entities are on the same line (`"resource provider"` and `info.id()`; `"resource"` and `resource`; opening and closing quotes).
```
LOG(INFO)
<< "Reconciling storage pools for resource provider " << info.id()
<< " after resource '" << resource << "' has been freed";
- Benjamin Bannier
On March 27, 2019, 4:45 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70314/
> -----------------------------------------------------------
>
> (Updated March 27, 2019, 4:45 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
> If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
> capability, this operation will be a no-op; otherwise the underlying CSI
> volume will be deprovisioned.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
>
>
> Diff: https://reviews.apache.org/r/70314/diff/2/
>
>
> Testing
> -------
>
> make check
>
> The new code path is tested later in this chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70314: Supported destroying preprovisioned CSI
volumes in SLRP.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70314/
-----------------------------------------------------------
(Updated March 28, 2019, 5:11 a.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-9540
https://issues.apache.org/jira/browse/MESOS-9540
Repository: mesos
Description
-------
SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
capability, this operation will be a no-op; otherwise the underlying CSI
volume will be deprovisioned.
Diffs (updated)
-----
src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
Diff: https://reviews.apache.org/r/70314/diff/3/
Changes: https://reviews.apache.org/r/70314/diff/2-3/
Testing
-------
make check
The new code path is tested later in this chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 70314: Supported destroying preprovisioned CSI
volumes in SLRP.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70314/
-----------------------------------------------------------
(Updated March 27, 2019, 3:45 a.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Removed an unnecessary `os::exists` check.
Bugs: MESOS-9540
https://issues.apache.org/jira/browse/MESOS-9540
Repository: mesos
Description
-------
SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
capability, this operation will be a no-op; otherwise the underlying CSI
volume will be deprovisioned.
Diffs (updated)
-----
src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
Diff: https://reviews.apache.org/r/70314/diff/2/
Changes: https://reviews.apache.org/r/70314/diff/1-2/
Testing
-------
make check
The new code path is tested later in this chain.
Thanks,
Chun-Hung Hsiao