You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/12/05 22:09:00 UTC
Review Request 64352: Added default VolumeProfile module
implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64352/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Jie Yu.
Bugs: MESOS-8251
https://issues.apache.org/jira/browse/MESOS-8251
Repository: mesos
Description
-------
By default, Mesos and the Storage Local Resource Provider (SLRP) will
not expect profiles to be used. This is partly due to the experimental
nature of the "profile" field and partly because some explicit operator
intervention is required to get profiles defined. There is no default
profile that is accepted by CSI plugins.
Diffs
-----
include/mesos/module/volume_profile.hpp PRE-CREATION
src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
src/resource_provider/volume_profile.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64352/diff/1/
Testing
-------
See end of chain.
Thanks,
Joseph Wu
Re: Review Request 64352: Added default VolumeProfile module
implementation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64352/#review193989
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Dec. 15, 2017, 7:28 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64352/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2017, 7:28 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
>
>
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By default, Mesos and the Storage Local Resource Provider (SLRP) will
> not expect profiles to be used. This is partly due to the experimental
> nature of the "profile" field and partly because some explicit operator
> intervention is required to get profiles defined. There is no default
> profile that is accepted by CSI plugins.
>
>
> Diffs
> -----
>
> include/mesos/module/volume_profile.hpp PRE-CREATION
> src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9
> src/module/manager.cpp d93648ab3f95e17d28056e9f5eb75ebff30775e1
> src/resource_provider/volume_profile.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64352/diff/3/
>
>
> Testing
> -------
>
> See end of chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 64352: Added default VolumeProfile module
implementation.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64352/
-----------------------------------------------------------
(Updated Dec. 15, 2017, 11:28 a.m.)
Review request for mesos, Chun-Hung Hsiao and Jie Yu.
Changes
-------
Added some missing module manager init.
Bugs: MESOS-8251
https://issues.apache.org/jira/browse/MESOS-8251
Repository: mesos
Description
-------
By default, Mesos and the Storage Local Resource Provider (SLRP) will
not expect profiles to be used. This is partly due to the experimental
nature of the "profile" field and partly because some explicit operator
intervention is required to get profiles defined. There is no default
profile that is accepted by CSI plugins.
Diffs (updated)
-----
include/mesos/module/volume_profile.hpp PRE-CREATION
src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9
src/module/manager.cpp d93648ab3f95e17d28056e9f5eb75ebff30775e1
src/resource_provider/volume_profile.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64352/diff/3/
Changes: https://reviews.apache.org/r/64352/diff/2-3/
Testing
-------
See end of chain.
Thanks,
Joseph Wu
Re: Review Request 64352: Added default VolumeProfile module
implementation.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64352/
-----------------------------------------------------------
(Updated Dec. 7, 2017, 2:08 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jie Yu.
Changes
-------
Updated to match changed interface (rename + extra parameter in both module functions).
Bugs: MESOS-8251
https://issues.apache.org/jira/browse/MESOS-8251
Repository: mesos
Description
-------
By default, Mesos and the Storage Local Resource Provider (SLRP) will
not expect profiles to be used. This is partly due to the experimental
nature of the "profile" field and partly because some explicit operator
intervention is required to get profiles defined. There is no default
profile that is accepted by CSI plugins.
Diffs (updated)
-----
include/mesos/module/volume_profile.hpp PRE-CREATION
src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
src/resource_provider/volume_profile.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64352/diff/2/
Changes: https://reviews.apache.org/r/64352/diff/1-2/
Testing
-------
See end of chain.
Thanks,
Joseph Wu
Re: Review Request 64352: Added default VolumeProfile module
implementation.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 6, 2017, 10:56 a.m., Jie Yu wrote:
> > src/resource_provider/volume_profile.cpp
> > Lines 53-54 (patched)
> > <https://reviews.apache.org/r/64352/diff/1/?file=1909245#file1909245line53>
> >
> > See my comments in the previous patch regarding default profile.
> >
> > For default volume profile adaptor, if `profile` is None(), simply return None() indicating that default profile is not supported (rather than returning a failure)?
> >
> > That means no resource will be sent out using default volume profile adaptor?
> >
> > That makes me wondering why we need a default volume provider adaptor. Can you just make the uri voluem profiler adaptor the default?
The default implementation is meant to be used with no configuration changes (i.e. if you don't know about or don't want to use the new feature). That's why it returns a Failure, as you can't use profiles if you haven't configured the agent to know about profiles.
If we make the URI module the default, we would need to add some required agent flags to specify the URI to fetch from and expose the other module flags too.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64352/#review193017
-----------------------------------------------------------
On Dec. 5, 2017, 2:09 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64352/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2017, 2:09 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
>
>
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By default, Mesos and the Storage Local Resource Provider (SLRP) will
> not expect profiles to be used. This is partly due to the experimental
> nature of the "profile" field and partly because some explicit operator
> intervention is required to get profiles defined. There is no default
> profile that is accepted by CSI plugins.
>
>
> Diffs
> -----
>
> include/mesos/module/volume_profile.hpp PRE-CREATION
> src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
> src/resource_provider/volume_profile.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64352/diff/1/
>
>
> Testing
> -------
>
> See end of chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 64352: Added default VolumeProfile module
implementation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64352/#review193017
-----------------------------------------------------------
src/resource_provider/volume_profile.cpp
Lines 53-54 (patched)
<https://reviews.apache.org/r/64352/#comment271503>
See my comments in the previous patch regarding default profile.
For default volume profile adaptor, if `profile` is None(), simply return None() indicating that default profile is not supported (rather than returning a failure)?
That means no resource will be sent out using default volume profile adaptor?
That makes me wondering why we need a default volume provider adaptor. Can you just make the uri voluem profiler adaptor the default?
- Jie Yu
On Dec. 5, 2017, 10:09 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64352/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2017, 10:09 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
>
>
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By default, Mesos and the Storage Local Resource Provider (SLRP) will
> not expect profiles to be used. This is partly due to the experimental
> nature of the "profile" field and partly because some explicit operator
> intervention is required to get profiles defined. There is no default
> profile that is accepted by CSI plugins.
>
>
> Diffs
> -----
>
> include/mesos/module/volume_profile.hpp PRE-CREATION
> src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
> src/resource_provider/volume_profile.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64352/diff/1/
>
>
> Testing
> -------
>
> See end of chain.
>
>
> Thanks,
>
> Joseph Wu
>
>