You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2017/11/14 14:11:56 UTC
Review Request 63798: Added resource provider support for all offer
operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Jie Yu.
Bugs: MESOS-8218
https://issues.apache.org/jira/browse/MESOS-8218
Repository: mesos
Description
-------
The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.
Diffs
-----
src/slave/slave.cpp d8bacebc74790e955490a158c37ac0d9e75fd6b5
src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
Diff: https://reviews.apache.org/r/63798/diff/1/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/#review190974
-----------------------------------------------------------
src/slave/slave.cpp
Lines 3756-3765 (patched)
<https://reviews.apache.org/r/63798/#comment268566>
Hum, looks like this is common for both agent default resources or resource provider resources. Can we do that in `addOfferOperation` (like we did in the master)?
Then, that'll break the following code for agent default resources :( Maybe it's better to just not re-use the same `checkpointResources` handler?
Please do more thinking here.
- Jie Yu
On Nov. 14, 2017, 2:11 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63798/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2017, 2:11 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8218
> https://issues.apache.org/jira/browse/MESOS-8218
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
> reservations and persistent volumes have been enabled for resource
> provider resources. Similar to agent resources, they are speculatively
> applied to allow offer pipelining.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp d8bacebc74790e955490a158c37ac0d9e75fd6b5
> src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
>
>
> Diff: https://reviews.apache.org/r/63798/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/#review191768
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Nov. 22, 2017, 12:15 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63798/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 12:15 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8218
> https://issues.apache.org/jira/browse/MESOS-8218
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
> reservations and persistent volumes have been enabled for resource
> provider resources. Similar to agent resources, they are speculatively
> applied to allow offer pipelining.
>
>
> Diffs
> -----
>
> src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c
> src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
> src/slave/slave.hpp f86ebd6b28ba2f6dfbf5acc5371af1573d5a4e96
> src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52
> src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
> src/tests/slave_tests.cpp 6033c7a674b12fc81bae28d73ed67d655b2b046c
>
>
> Diff: https://reviews.apache.org/r/63798/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/
-----------------------------------------------------------
(Updated Nov. 22, 2017, 1:15 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed issues.
Bugs: MESOS-8218
https://issues.apache.org/jira/browse/MESOS-8218
Repository: mesos
Description
-------
The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.
Diffs (updated)
-----
src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c
src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
src/slave/slave.hpp f86ebd6b28ba2f6dfbf5acc5371af1573d5a4e96
src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52
src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
src/tests/slave_tests.cpp 6033c7a674b12fc81bae28d73ed67d655b2b046c
Diff: https://reviews.apache.org/r/63798/diff/5/
Changes: https://reviews.apache.org/r/63798/diff/4-5/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/#review191639
-----------------------------------------------------------
src/slave/slave.cpp
Lines 3532 (patched)
<https://reviews.apache.org/r/63798/#comment269481>
This is problematic. What if it's an old master but a new agent? old master will still send checkpointed resources message, and we'll miss updating the total in that case.
I better approach is to pass an optional parameter to this method and let the caller control that.
src/tests/resource_provider_manager_tests.cpp
Line 805 (original), 713 (patched)
<https://reviews.apache.org/r/63798/#comment269496>
Do you want to have an ASSERT here checking if `resources` is not empty?
- Jie Yu
On Nov. 21, 2017, 3:30 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63798/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 3:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8218
> https://issues.apache.org/jira/browse/MESOS-8218
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
> reservations and persistent volumes have been enabled for resource
> provider resources. Similar to agent resources, they are speculatively
> applied to allow offer pipelining.
>
>
> Diffs
> -----
>
> src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c
> src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
> src/slave/slave.cpp 6e9adc60f593faf1b0e56caeea04882f67af7080
> src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
>
>
> Diff: https://reviews.apache.org/r/63798/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/
-----------------------------------------------------------
(Updated Nov. 21, 2017, 4:30 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Refactored to use default behavior of `MockResourceProvider`.
Bugs: MESOS-8218
https://issues.apache.org/jira/browse/MESOS-8218
Repository: mesos
Description
-------
The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.
Diffs (updated)
-----
src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c
src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
src/slave/slave.cpp 6e9adc60f593faf1b0e56caeea04882f67af7080
src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
Diff: https://reviews.apache.org/r/63798/diff/4/
Changes: https://reviews.apache.org/r/63798/diff/3-4/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/
-----------------------------------------------------------
(Updated Nov. 16, 2017, 3:43 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed issues.
Bugs: MESOS-8218
https://issues.apache.org/jira/browse/MESOS-8218
Repository: mesos
Description
-------
The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.
Diffs (updated)
-----
src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c
src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
Diff: https://reviews.apache.org/r/63798/diff/3/
Changes: https://reviews.apache.org/r/63798/diff/2-3/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/#review191122
-----------------------------------------------------------
src/slave/slave.cpp
Line 3737 (original), 3737 (patched)
<https://reviews.apache.org/r/63798/#comment268716>
Can you update `needCheckpointing` to just look at agent default resources?
src/slave/slave.cpp
Lines 3737-3749 (original), 3737-3742 (patched)
<https://reviews.apache.org/r/63798/#comment268720>
As I mentioned earlier, `chckpointResources` will overwrite `totalResources`, which is wrong with resource provider resources.
src/tests/resource_provider_manager_tests.cpp
Lines 810-811 (original), 833-834 (patched)
<https://reviews.apache.org/r/63798/#comment268723>
This does not sound like a valid sequence. I'm surprised that we allow CREATE on a RAW disk resources. Can you add some validation to disallow that?
Another suggestion is that we should add some default action to MockResourceProvider so that we can reduce the size of this test. The default behavior for handling an operation is to just return the converted resources (for new operations), and success for old operations. One can override the default behavior. Take a look at `TestAllocator`
- Jie Yu
On Nov. 15, 2017, 11:35 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63798/
> -----------------------------------------------------------
>
> (Updated Nov. 15, 2017, 11:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8218
> https://issues.apache.org/jira/browse/MESOS-8218
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
> reservations and persistent volumes have been enabled for resource
> provider resources. Similar to agent resources, they are speculatively
> applied to allow offer pipelining.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
> src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
>
>
> Diff: https://reviews.apache.org/r/63798/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63798: Added resource provider support for all
offer operations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63798/
-----------------------------------------------------------
(Updated Nov. 15, 2017, 12:35 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed issues.
Bugs: MESOS-8218
https://issues.apache.org/jira/browse/MESOS-8218
Repository: mesos
Description
-------
The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.
Diffs (updated)
-----
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
src/tests/resource_provider_manager_tests.cpp ecfe2b4c0952838d6312df603f8eb2f458725175
Diff: https://reviews.apache.org/r/63798/diff/2/
Changes: https://reviews.apache.org/r/63798/diff/1-2/
Testing
-------
make check
Thanks,
Jan Schlicht