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/22 06:18:40 UTC
Re: Review Request 70215: Cleanup volume attaching and publishing for
SLRP.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70215/
-----------------------------------------------------------
(Updated March 22, 2019, 6:18 a.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed Benjamin's comments and restructured the refactoring.
Summary (updated)
-----------------
Cleanup volume attaching and publishing for SLRP.
Bugs: MESOS-9622
https://issues.apache.org/jira/browse/MESOS-9622
Repository: mesos
Description (updated)
-------
This patch introduces methods for volume attaching and publishing that
conform to `VolumeManager`'s public interface in SLRP, and cleans up
SLRP based on these functions. They will be moved out from SLRP to v0
`VolumeManager` later.
Volume attaching and publishing are implemented with internal helpers,
each individually deals with one steady and two transient states, and
makes a proper CSI call to achieve its goal state. If the given volume
is not in the designed state, it would recursively call other helpers
to transition the volume to the designed state first.
These helper functions are serialized through the volume's own sequence
to avoid racing operations on the same volume.
Diffs (updated)
-----
src/resource_provider/storage/provider.cpp fea623c292158deb1b4b4b9ab1ac208031471519
src/resource_provider/storage/provider_process.hpp a5536b3d735e01eb1c4dc52d0602d973155f3c93
Diff: https://reviews.apache.org/r/70215/diff/3/
Changes: https://reviews.apache.org/r/70215/diff/2-3/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70215: Cleanup volume attaching and publishing for
SLRP.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70215/
-----------------------------------------------------------
(Updated March 28, 2019, 7:55 a.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-9622
https://issues.apache.org/jira/browse/MESOS-9622
Repository: mesos
Description
-------
This patch introduces methods for volume attaching and publishing that
conform to `VolumeManager`'s public interface in SLRP, and cleans up
SLRP based on these functions. They will be moved out from SLRP to v0
`VolumeManager` later.
Volume attaching and publishing are implemented with internal helpers,
each individually deals with one steady and two transient states, and
makes a proper CSI call to achieve its goal state. If the given volume
is not in the designed state, it would recursively call other helpers
to transition the volume to the designed state first.
These helper functions are serialized through the volume's own sequence
to avoid racing operations on the same volume.
Diffs (updated)
-----
src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
src/resource_provider/storage/provider_process.hpp a5536b3d735e01eb1c4dc52d0602d973155f3c93
Diff: https://reviews.apache.org/r/70215/diff/6/
Changes: https://reviews.apache.org/r/70215/diff/5-6/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70215: Cleanup volume attaching and publishing for
SLRP.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70215/
-----------------------------------------------------------
(Updated March 27, 2019, 5:58 a.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Rebased.
Bugs: MESOS-9622
https://issues.apache.org/jira/browse/MESOS-9622
Repository: mesos
Description
-------
This patch introduces methods for volume attaching and publishing that
conform to `VolumeManager`'s public interface in SLRP, and cleans up
SLRP based on these functions. They will be moved out from SLRP to v0
`VolumeManager` later.
Volume attaching and publishing are implemented with internal helpers,
each individually deals with one steady and two transient states, and
makes a proper CSI call to achieve its goal state. If the given volume
is not in the designed state, it would recursively call other helpers
to transition the volume to the designed state first.
These helper functions are serialized through the volume's own sequence
to avoid racing operations on the same volume.
Diffs (updated)
-----
src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
src/resource_provider/storage/provider_process.hpp a5536b3d735e01eb1c4dc52d0602d973155f3c93
Diff: https://reviews.apache.org/r/70215/diff/5/
Changes: https://reviews.apache.org/r/70215/diff/4-5/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70215: Cleanup volume attaching and publishing for
SLRP.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On March 25, 2019, 3:37 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1670 (original), 1620 (patched)
> > <https://reviews.apache.org/r/70215/diff/4/?file=2133679#file2133679line1691>
> >
> > Do we need to `CHECK`? Or can the volume legitimately go away while we `defer` and should return instead here?
> >
> > I'd in general prefer returning something over a `CHECK` as it introduces less coupling.
No. The volume should not go away. This is an invariant I'd like to maintain so I'll add a check here.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70215/#review213981
-----------------------------------------------------------
On March 27, 2019, 5:58 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70215/
> -----------------------------------------------------------
>
> (Updated March 27, 2019, 5:58 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
> -------
>
> This patch introduces methods for volume attaching and publishing that
> conform to `VolumeManager`'s public interface in SLRP, and cleans up
> SLRP based on these functions. They will be moved out from SLRP to v0
> `VolumeManager` later.
>
> Volume attaching and publishing are implemented with internal helpers,
> each individually deals with one steady and two transient states, and
> makes a proper CSI call to achieve its goal state. If the given volume
> is not in the designed state, it would recursively call other helpers
> to transition the volume to the designed state first.
>
> These helper functions are serialized through the volume's own sequence
> to avoid racing operations on the same volume.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af
> src/resource_provider/storage/provider_process.hpp a5536b3d735e01eb1c4dc52d0602d973155f3c93
>
>
> Diff: https://reviews.apache.org/r/70215/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70215: Cleanup volume attaching and publishing for
SLRP.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70215/#review213981
-----------------------------------------------------------
Fix it, then Ship it!
src/resource_provider/storage/provider.cpp
Line 1637 (original), 1569 (patched)
<https://reviews.apache.org/r/70215/#comment300124>
We could move these right after `StorageLocalResourceProviderProcess::attachVolume` like we also do in e.g., `src/master/master.cpp`. Here and below.
OTOH, this code is going away soon, so not marking this as an issue.
src/resource_provider/storage/provider.cpp
Line 1640 (original), 1572 (patched)
<https://reviews.apache.org/r/70215/#comment300125>
This was here before and seems to work, but I wonder if it wouldn't be easier to follow if we'd return a `Failure` here instead as it would be immediately clear that the code would be safe.
Also below.
src/resource_provider/storage/provider.cpp
Line 1670 (original), 1620 (patched)
<https://reviews.apache.org/r/70215/#comment300134>
Do we need to `CHECK`? Or can the volume legitimately go away while we `defer` and should return instead here?
I'd in general prefer returning something over a `CHECK` as it introduces less coupling.
src/resource_provider/storage/provider.cpp
Line 1776 (original), 1754 (patched)
<https://reviews.apache.org/r/70215/#comment300135>
Ditto.
src/resource_provider/storage/provider.cpp
Line 1899 (original), 1896 (patched)
<https://reviews.apache.org/r/70215/#comment300136>
Ditto.
- Benjamin Bannier
On March 22, 2019, 7:18 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70215/
> -----------------------------------------------------------
>
> (Updated March 22, 2019, 7:18 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
> -------
>
> This patch introduces methods for volume attaching and publishing that
> conform to `VolumeManager`'s public interface in SLRP, and cleans up
> SLRP based on these functions. They will be moved out from SLRP to v0
> `VolumeManager` later.
>
> Volume attaching and publishing are implemented with internal helpers,
> each individually deals with one steady and two transient states, and
> makes a proper CSI call to achieve its goal state. If the given volume
> is not in the designed state, it would recursively call other helpers
> to transition the volume to the designed state first.
>
> These helper functions are serialized through the volume's own sequence
> to avoid racing operations on the same volume.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/provider.cpp fea623c292158deb1b4b4b9ab1ac208031471519
> src/resource_provider/storage/provider_process.hpp a5536b3d735e01eb1c4dc52d0602d973155f3c93
>
>
> Diff: https://reviews.apache.org/r/70215/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>