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...@mesosphere.io> on 2017/12/01 01:55:16 UTC
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/
-----------------------------------------------------------
(Updated Dec. 1, 2017, 1:55 a.m.)
Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
Changes
-------
Added `VolumeState` protobuf.
Bugs: MESOS-8143
https://issues.apache.org/jira/browse/MESOS-8143
Repository: mesos
Description (updated)
-------
Storage local resource provider can now handle PUBLISH events.
Although we don't do UNPUBLISH for now, the unpublish function is
still required for deleting volumes.
This patch also adds a protobuf to keep track of CSI volume states,
which persist across agents.
Diffs (updated)
-----
src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
src/csi/state.hpp PRE-CREATION
src/csi/state.proto PRE-CREATION
src/csi/utils.hpp 8ee97b2192d00f7e266c297de49315b5ce377308
src/csi/utils.cpp 4dd21fbae2070606c36c94415957ea3213a7dad1
src/resource_provider/storage/provider.cpp f586afc256fbcb2f2bf2451ffb7e7ba1d59cfa78
Diff: https://reviews.apache.org/r/63387/diff/4/
Changes: https://reviews.apache.org/r/63387/diff/3-4/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/
-----------------------------------------------------------
(Updated Dec. 7, 2017, 12:21 a.m.)
Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
Changes
-------
Rebased.
Bugs: MESOS-8143
https://issues.apache.org/jira/browse/MESOS-8143
Repository: mesos
Description
-------
Storage local resource provider can now handle PUBLISH events.
Although we don't do UNPUBLISH for now, the unpublish function is
still required for deleting volumes.
Diffs (updated)
-----
src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
src/csi/state.hpp PRE-CREATION
src/csi/state.proto PRE-CREATION
src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5
src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18
src/resource_provider/storage/provider.cpp 256b214b1d31b50fb1812227ae10a387a66e87e0
Diff: https://reviews.apache.org/r/63387/diff/8/
Changes: https://reviews.apache.org/r/63387/diff/7-8/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/
-----------------------------------------------------------
(Updated Dec. 6, 2017, 8:01 p.m.)
Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-8143
https://issues.apache.org/jira/browse/MESOS-8143
Repository: mesos
Description
-------
Storage local resource provider can now handle PUBLISH events.
Although we don't do UNPUBLISH for now, the unpublish function is
still required for deleting volumes.
Diffs (updated)
-----
src/Makefile.am d5ca797c5a57a2763374d391a7e9358fe29f5e06
src/csi/state.hpp PRE-CREATION
src/csi/state.proto PRE-CREATION
src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5
src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18
src/resource_provider/storage/provider.cpp d35b0d02992e3730ca47906b34c21e1ba9c653e7
Diff: https://reviews.apache.org/r/63387/diff/7/
Changes: https://reviews.apache.org/r/63387/diff/6-7/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 696-698 (patched)
> > <https://reviews.apache.org/r/63387/diff/6/?file=1908770#file1908770line696>
> >
> > Can you comment on why this is not a fatal error and it is safe to ignore?
I'll follow the error handling we have for `ResourceProviderState` checkpointing.
> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 749-752 (patched)
> > <https://reviews.apache.org/r/63387/diff/6/?file=1908770#file1908770line749>
> >
> > Why do you need this? Can you add a `default` and set it to UNREACHABLE?
https://github.com/google/protobuf/issues/3917
> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 962-965 (patched)
> > <https://reviews.apache.org/r/63387/diff/6/?file=1908770#file1908770line962>
> >
> > Can this just be `default`?
Ditto.
> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1578-1582 (patched)
> > <https://reviews.apache.org/r/63387/diff/6/?file=1908770#file1908770line1578>
> >
> > Do you need this?
No I don't need. Missed this during refactoring :(
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/#review192946
-----------------------------------------------------------
On Dec. 5, 2017, 5:59 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63387/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2017, 5:59 a.m.)
>
>
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
>
>
> Bugs: MESOS-8143
> https://issues.apache.org/jira/browse/MESOS-8143
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Storage local resource provider can now handle PUBLISH events.
> Although we don't do UNPUBLISH for now, the unpublish function is
> still required for deleting volumes.
>
>
> Diffs
> -----
>
> src/Makefile.am d5ca797c5a57a2763374d391a7e9358fe29f5e06
> src/csi/state.hpp PRE-CREATION
> src/csi/state.proto PRE-CREATION
> src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5
> src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18
> src/resource_provider/storage/provider.cpp d35b0d02992e3730ca47906b34c21e1ba9c653e7
>
>
> Diff: https://reviews.apache.org/r/63387/diff/6/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/#review192946
-----------------------------------------------------------
Fix it, then Ship it!
src/resource_provider/storage/provider.cpp
Lines 310 (patched)
<https://reviews.apache.org/r/63387/#comment271334>
Can you add a comment about why `sequence` is needed?
src/resource_provider/storage/provider.cpp
Lines 668 (patched)
<https://reviews.apache.org/r/63387/#comment271314>
attach the failure?
src/resource_provider/storage/provider.cpp
Lines 696-698 (patched)
<https://reviews.apache.org/r/63387/#comment271315>
Can you comment on why this is not a fatal error and it is safe to ignore?
src/resource_provider/storage/provider.cpp
Lines 749-752 (patched)
<https://reviews.apache.org/r/63387/#comment271321>
Why do you need this? Can you add a `default` and set it to UNREACHABLE?
src/resource_provider/storage/provider.cpp
Lines 932-933 (patched)
<https://reviews.apache.org/r/63387/#comment271336>
What if there are multiple resources (e.g, PATH) disk that points to the same volume id? Can you add some comments about that?
src/resource_provider/storage/provider.cpp
Lines 933 (patched)
<https://reviews.apache.org/r/63387/#comment271332>
Can you add a CHECK here asserting that the resource is a disk resource with an ID?
src/resource_provider/storage/provider.cpp
Lines 962-965 (patched)
<https://reviews.apache.org/r/63387/#comment271337>
Can this just be `default`?
src/resource_provider/storage/provider.cpp
Lines 1578-1582 (patched)
<https://reviews.apache.org/r/63387/#comment271325>
Do you need this?
src/resource_provider/storage/provider.cpp
Lines 1636-1640 (patched)
<https://reviews.apache.org/r/63387/#comment271326>
DO you need this?
src/resource_provider/storage/provider.cpp
Lines 1704-1708 (patched)
<https://reviews.apache.org/r/63387/#comment271327>
Do you need this?
- Jie Yu
On Dec. 5, 2017, 5:59 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63387/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2017, 5:59 a.m.)
>
>
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
>
>
> Bugs: MESOS-8143
> https://issues.apache.org/jira/browse/MESOS-8143
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Storage local resource provider can now handle PUBLISH events.
> Although we don't do UNPUBLISH for now, the unpublish function is
> still required for deleting volumes.
>
>
> Diffs
> -----
>
> src/Makefile.am d5ca797c5a57a2763374d391a7e9358fe29f5e06
> src/csi/state.hpp PRE-CREATION
> src/csi/state.proto PRE-CREATION
> src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5
> src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18
> src/resource_provider/storage/provider.cpp d35b0d02992e3730ca47906b34c21e1ba9c653e7
>
>
> Diff: https://reviews.apache.org/r/63387/diff/6/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/
-----------------------------------------------------------
(Updated Dec. 5, 2017, 5:59 a.m.)
Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
Changes
-------
Fixed a bug for maintaining volume states of new volumes.
Bugs: MESOS-8143
https://issues.apache.org/jira/browse/MESOS-8143
Repository: mesos
Description
-------
Storage local resource provider can now handle PUBLISH events.
Although we don't do UNPUBLISH for now, the unpublish function is
still required for deleting volumes.
Diffs (updated)
-----
src/Makefile.am d5ca797c5a57a2763374d391a7e9358fe29f5e06
src/csi/state.hpp PRE-CREATION
src/csi/state.proto PRE-CREATION
src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5
src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18
src/resource_provider/storage/provider.cpp d35b0d02992e3730ca47906b34c21e1ba9c653e7
Diff: https://reviews.apache.org/r/63387/diff/6/
Changes: https://reviews.apache.org/r/63387/diff/5-6/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 63387: Added publish/unpublish in storage local
resource provider.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63387/
-----------------------------------------------------------
(Updated Dec. 5, 2017, 2:19 a.m.)
Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
Bugs: MESOS-8143
https://issues.apache.org/jira/browse/MESOS-8143
Repository: mesos
Description (updated)
-------
Storage local resource provider can now handle PUBLISH events.
Although we don't do UNPUBLISH for now, the unpublish function is
still required for deleting volumes.
Diffs (updated)
-----
src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b
src/csi/state.hpp PRE-CREATION
src/csi/state.proto PRE-CREATION
src/csi/utils.hpp 8ee97b2192d00f7e266c297de49315b5ce377308
src/csi/utils.cpp 4dd21fbae2070606c36c94415957ea3213a7dad1
src/resource_provider/storage/provider.cpp d35b0d02992e3730ca47906b34c21e1ba9c653e7
Diff: https://reviews.apache.org/r/63387/diff/5/
Changes: https://reviews.apache.org/r/63387/diff/4-5/
Testing
-------
make
Thanks,
Chun-Hung Hsiao