You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2018/03/13 22:48:26 UTC
Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965
Repository: mesos
Description
-------
Added offer operation to grow and shrink persistent volumes.
Diffs
-----
include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
Diff: https://reviews.apache.org/r/66049/diff/1/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On March 15, 2018, 7:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > <https://reviews.apache.org/r/66049/diff/1/?file=1974927#file1974927line1975>
> >
> > I'm thinking that, instead of asking the framework to craft the freed disk, we could leave it to the agent/RP so they have the freedom to transform the freed disk to an appropriate type of disk resource (although for now it will be the same as the original volume except in size and persistence). So how about having a `Scalar target` instead, and we can implement it through `Resources::shrink()`?
>
> Zhitao Li wrote:
> I suggest we do not look at how `Resources` class implements various helpers when designing public API, but rather think about how to make the API easy, straightforward and consistent for the users (here it would be framework authors).
>
> Initially, I chose this format because it is more symetrical to `RESERVE`/`UNRESERVE` and could be chained together in one operation. However, after the decision to make new API non-chainable (and non-speculative), that argument seems weaker now.
>
> If we go with `target` format, I'd rather make the target as a full `Resource` object and the API will look like:
>
> ```
> message SHRINK_VOLUME {
> required Resource original = 1; // original volume resource
> required Resource target = 2; // target volume resource
> }
> ```
>
> Framework author can just make a copy of `original` and change the scalar quantity.
>
> Eventually, we might be able to mark `original` optional or drop it in the API.
>
> What do you think?
Yeah you're right about not making API decision based on utility functions.
Dropping `original` doesn't sound a good idea to me for the following reasons:
1. Consistency-wise, it would be different from other non-speculative calls such as `CREATE_VOLUME` or `DESTROY_VOLUME` (NOTE: I'm not talking about the `CREATE_VOLUMES` or `DESTROY_VOLUMES` operator calls), where we specify the source resource and let the operation performer to craft the converted.results.
2. Implementation-wise, the master and agent still need to find the original resource to construct and apply the resource conversion. If that doesn't come from the request, we need to do an extra search to find the source, which I don't feel necessary.
For `target` scalar vs resource, we could think about what are the restrictions we'd like to enforce. If we want to make sure that the resulting shrunk resource must look exactly the same and have no other metadata change, then I'm fine with making it a `Reaource`. If we want to have minimal restrictions and give the operation performer some freedom to modify the resoure (for example, make `taeget` a reference size or upper limit and thus the agent can shrink the volume as small as possible), then I feel a scalar looks better. FYI, in `CREATE_VOLUME`, we specify a `source` and a target type (`MOUNT` or `PATH`) only instead of the resulting resource, because the reaource provider will fill in extra information such as `id` and `metadata`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/#review199283
-----------------------------------------------------------
On March 13, 2018, 10:48 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 10:48 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added offer operation to grow and shrink persistent volumes.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
> include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
>
>
> Diff: https://reviews.apache.org/r/66049/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Zhitao Li <zh...@apache.org>.
> On March 15, 2018, 12:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > <https://reviews.apache.org/r/66049/diff/1/?file=1974927#file1974927line1920>
> >
> > As we discussed, let's do `s/PERSISTENT_//`, as well as `s/Persistent//` and `s/persistent_//` below.
Will do.
> On March 15, 2018, 12:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > <https://reviews.apache.org/r/66049/diff/1/?file=1974927#file1974927line1975>
> >
> > I'm thinking that, instead of asking the framework to craft the freed disk, we could leave it to the agent/RP so they have the freedom to transform the freed disk to an appropriate type of disk resource (although for now it will be the same as the original volume except in size and persistence). So how about having a `Scalar target` instead, and we can implement it through `Resources::shrink()`?
I suggest we do not look at how `Resources` class implements various helpers when designing public API, but rather think about how to make the API easy, straightforward and consistent for the users (here it would be framework authors).
Initially, I chose this format because it is more symetrical to `RESERVE`/`UNRESERVE` and could be chained together in one operation. However, after the decision to make new API non-chainable (and non-speculative), that argument seems weaker now.
If we go with `target` format, I'd rather make the target as a full `Resource` object and the API will look like:
```
message SHRINK_VOLUME {
required Resource original = 1; // original volume resource
required Resource target = 2; // target volume resource
}
```
Framework author can just make a copy of `original` and change the scalar quantity.
Eventually, we might be able to mark `original` optional or drop it in the API.
What do you think?
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/#review199283
-----------------------------------------------------------
On March 13, 2018, 3:48 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 3:48 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added offer operation to grow and shrink persistent volumes.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
> include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
>
>
> Diff: https://reviews.apache.org/r/66049/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/#review199283
-----------------------------------------------------------
include/mesos/mesos.proto
Lines 1920-1921 (patched)
<https://reviews.apache.org/r/66049/#comment279588>
As we discussed, let's do `s/PERSISTENT_//`, as well as `s/Persistent//` and `s/persistent_//` below.
include/mesos/mesos.proto
Lines 1975 (patched)
<https://reviews.apache.org/r/66049/#comment279590>
I'm thinking that, instead of asking the framework to craft the freed disk, we could leave it to the agent/RP so they have the freedom to transform the freed disk to an appropriate type of disk resource (although for now it will be the same as the original volume except in size and persistence). So how about having a `Scalar target` instead, and we can implement it through `Resources::shrink()`?
- Chun-Hung Hsiao
On March 13, 2018, 10:48 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 10:48 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added offer operation to grow and shrink persistent volumes.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
> include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
>
>
> Diff: https://reviews.apache.org/r/66049/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/#review199354
-----------------------------------------------------------
include/mesos/mesos.proto
Lines 1967 (patched)
<https://reviews.apache.org/r/66049/#comment279663>
I'd suggest the following:
```
message GrowVolume {
required Resource volume = 1;
required Resource addition = 2;
}
```
Using `Resource` for `addition` make it clear that you need an offer containing the resources to be able to grow an volume. You cannot bindly grown a volume. The master will validate that `addition` is actually allocated to the framework.
The master will also need to validate that the `addition` Resource is compatible with `volume` in the operation.
1) If `volume` is a `ROOT` disk, `addition` has to be a `ROOT` disk too.
2) If `volume` is a `PATH` disk, the `addition` can either be a `PATH` disk with the same `Source`, or a `RAW` disk from the same resource provider and the same profile.
3) If `volume` is a `MOUNT` disk, the `addition` should be a `RAW` disk from the same resource provider and the same profile.
Given that, using a `Resource` for `addition` is more appropriate and future proof.
- Jie Yu
On March 16, 2018, 5:54 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> -----------------------------------------------------------
>
> (Updated March 16, 2018, 5:54 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added offer operation to grow and shrink persistent volumes.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
> include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
>
>
> Diff: https://reviews.apache.org/r/66049/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/#review199796
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/mesos.proto
Lines 1962 (patched)
<https://reviews.apache.org/r/66049/#comment280255>
How about "Grow a volume by an additional disk resource"? Ditto below.
include/mesos/mesos.proto
Lines 1970 (patched)
<https://reviews.apache.org/r/66049/#comment280260>
"Shrink a volume by size specified in the `subtract` field" here and below.
- Chun-Hung Hsiao
On March 21, 2018, 1:31 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> -----------------------------------------------------------
>
> (Updated March 21, 2018, 1:31 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added offer operation to grow and shrink persistent volumes.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
> include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
>
>
> Diff: https://reviews.apache.org/r/66049/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/
-----------------------------------------------------------
(Updated March 22, 2018, 3:36 p.m.)
Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
Changes
-------
Updated comment.
Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965
Repository: mesos
Description
-------
Added offer operation to grow and shrink persistent volumes.
Diffs (updated)
-----
include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907
include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95
Diff: https://reviews.apache.org/r/66049/diff/4/
Changes: https://reviews.apache.org/r/66049/diff/3-4/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/
-----------------------------------------------------------
(Updated March 20, 2018, 6:31 p.m.)
Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
Changes
-------
Updated API.
Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965
Repository: mesos
Description
-------
Added offer operation to grow and shrink persistent volumes.
Diffs (updated)
-----
include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
Diff: https://reviews.apache.org/r/66049/diff/3/
Changes: https://reviews.apache.org/r/66049/diff/2-3/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66049: Added offer operation to grow and shrink
persistent volumes.
Posted by Zhitao Li <zh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66049/
-----------------------------------------------------------
(Updated March 16, 2018, 10:54 a.m.)
Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965
Repository: mesos
Description
-------
Added offer operation to grow and shrink persistent volumes.
Diffs (updated)
-----
include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9
include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8
Diff: https://reviews.apache.org/r/66049/diff/2/
Changes: https://reviews.apache.org/r/66049/diff/1-2/
Testing
-------
Thanks,
Zhitao Li