Re: Review Request 70215: Cleanup volume attaching and publishing for SLRP.

2019-03-28 Thread Chun-Hung Hsiao 

---
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.

2019-03-28 Thread Chun-Hung Hsiao 


> On March 25, 2019, 3:37 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1670 (original), 1620 (patched)
> > 
> >
> > 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.

2019-03-26 Thread Chun-Hung Hsiao 

---
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.

2019-03-25 Thread Benjamin Bannier 

---
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)


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)


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)


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)


Ditto.



src/resource_provider/storage/provider.cpp
Line 1899 (original), 1896 (patched)


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
> 
>

Re: Review Request 70215: Cleanup volume attaching and publishing for SLRP.

2019-03-22 Thread Chun-Hung Hsiao 


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 146 (patched)
> > 
> >
> > `s/sequentialized/serialized/` here and below?
> > 
> > _attaching_ to be consistent with other methods?

Fixed in r/70285.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 159 (patched)
> > 
> >
> > _detaching_?

Fixed in r/70285.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 185 (patched)
> > 
> >
> > _unpublishing_?

Fixed in r/70285.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 192 (patched)
> > 
> >
> > Let's use uppercase initials for even for non-type template parameters, 
> > `s/rpc/Rpc/g`.
> > 
> > Here and below.

Fixed in r/70222.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 202 (patched)
> > 
> >
> > Break line before `.then`.

Fixed in r/70222.


- Chun-Hung


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


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

Re: Review Request 70215: Cleanup volume attaching and publishing for SLRP.

2019-03-22 Thread Chun-Hung Hsiao 

---
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