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/13 14:21:09 UTC
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 13, 2017, 3:21 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Fixed an issues with allocation info being set.
Summary (updated)
-----------------
Triggered 'ApplyOfferOperationMessage' for agent local resources.
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 0ca4c6d689bf7d2c477174b039432ed7b6d0b650
src/common/protobuf_utils.cpp 5739a63f8d87923c034375b88c4f0b3b19f4b521
src/master/master.cpp 49dbaa979d692061a7b479b1db4511e8357b8baf
src/slave/slave.cpp 7cb6661b55fb5437a1ffc447f974076aadd1eced
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/2/
Changes: https://reviews.apache.org/r/63751/diff/1-2/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review190841
-----------------------------------------------------------
PASS: Mesos patch 63751 was successfully built and tested.
Reviews applied: `['63622', '63679', '63625', '63751']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63751
- Mesos Reviewbot Windows
On Nov. 13, 2017, 2:21 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2017, 2:21 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 0ca4c6d689bf7d2c477174b039432ed7b6d0b650
> src/common/protobuf_utils.cpp 5739a63f8d87923c034375b88c4f0b3b19f4b521
> src/master/master.cpp 49dbaa979d692061a7b479b1db4511e8357b8baf
> src/slave/slave.cpp 7cb6661b55fb5437a1ffc447f974076aadd1eced
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review191766
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/slave.cpp
Lines 3763-3764 (patched)
<https://reviews.apache.org/r/63751/#comment269644>
THis should be `updateOfferOperation(update);`
- Jie Yu
On Nov. 22, 2017, 12:11 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 12:11 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
> src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
> src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
> src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52
> src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/8/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 22, 2017, 1:11 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed issues.
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52
src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/8/
Changes: https://reviews.apache.org/r/63751/diff/7-8/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 21, 2017, 4:28 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Fixed a small issue.
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
src/slave/slave.cpp 6e9adc60f593faf1b0e56caeea04882f67af7080
src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/7/
Changes: https://reviews.apache.org/r/63751/diff/6-7/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 17, 2017, 3 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Added a helper `isSpeculativeOperation`.
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/6/
Changes: https://reviews.apache.org/r/63751/diff/5-6/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 16, 2017, 7:59 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Fixed a race condition in the parameterized tests (Offers have to be sent out after `UpdateSlaveMessage` was handled in the agent).
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/5/
Changes: https://reviews.apache.org/r/63751/diff/4-5/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Nov. 15, 2017, 10:53 p.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 3751-3757 (patched)
> > <https://reviews.apache.org/r/63751/diff/4/?file=1893429#file1893429line3751>
> >
> > Are we leaking `OfferOperation` in the agent memory if we don't call `updateOfferOperation` and then `removeOfferOperation`?
I've changed the code to remove speculatively applied offer operation after sending an update to the master.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review191109
-----------------------------------------------------------
On Nov. 22, 2017, 1:11 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 1:11 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
> src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
> src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
> src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52
> src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/8/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review191109
-----------------------------------------------------------
src/slave/slave.cpp
Lines 3751-3757 (patched)
<https://reviews.apache.org/r/63751/#comment268699>
Are we leaking `OfferOperation` in the agent memory if we don't call `updateOfferOperation` and then `removeOfferOperation`?
- Jie Yu
On Nov. 15, 2017, 4:06 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 15, 2017, 4:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
> src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
> src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
> src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Nov. 16, 2017, 12:37 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 3743 (patched)
> > <https://reviews.apache.org/r/63751/diff/4/?file=1893429#file1893429line3743>
> >
> > For old operations, I think we should apply the conversion (i.e., change `totalResources`) in `addOfferOperation` becuase we speculatively assume that it'll succeeds.
> >
> > This makes sense for old operation that has a resource provider. The current code is buggy because we don't update agent `totalResources` for those old operations.
> >
> > Then, you don't have to do this assignment here anymore. We still need to calculate the checkpointed resources so that we can call the old checkpointResources handler.
> >
> > One unfortunate thing is that `checkpointResources` method will also set `totalResources`. This is in fact a bug:
> >
> > ```
> > // This is a sanity check to verify that the new checkpointed
> > // resources are compatible with the agent resources specified
> > // through the '--resources' command line flag. The resources
> > // should be guaranteed compatible by the master.
> > Try<Resources> _totalResources = applyCheckpointedResources(
> > info.resources(),
> > newCheckpointedResources);
> >
> > CHECK_SOME(_totalResources)
> > << "Failed to apply checkpointed resources "
> > << newCheckpointedResources << " to agent's resources "
> > << info.resources();
> >
> > totalResources = _totalResources.get();
> > ```
> >
> > Setting agent resources to `_totalResources.get()` will break the agent resource accounting (if resource provider has resources). We might need to introduce a parameter to `checkpointResources` to not update `totalResources`.
This is addressed in #63798.
> On Nov. 16, 2017, 12:37 a.m., Jie Yu wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/63751/diff/4/?file=1893430#file1893430line94>
> >
> > Can you add a comment about what that `bool` represents? Ditto in ReservationTest
Changed it to an enumeration, as that is more clear than a boolean and can be named.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review191114
-----------------------------------------------------------
On Nov. 22, 2017, 1:11 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 1:11 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
> src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
> src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
> src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52
> src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/8/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review191114
-----------------------------------------------------------
src/slave/slave.cpp
Lines 3743 (patched)
<https://reviews.apache.org/r/63751/#comment268701>
For old operations, I think we should apply the conversion (i.e., change `totalResources`) in `addOfferOperation` becuase we speculatively assume that it'll succeeds.
This makes sense for old operation that has a resource provider. The current code is buggy because we don't update agent `totalResources` for those old operations.
Then, you don't have to do this assignment here anymore. We still need to calculate the checkpointed resources so that we can call the old checkpointResources handler.
One unfortunate thing is that `checkpointResources` method will also set `totalResources`. This is in fact a bug:
```
// This is a sanity check to verify that the new checkpointed
// resources are compatible with the agent resources specified
// through the '--resources' command line flag. The resources
// should be guaranteed compatible by the master.
Try<Resources> _totalResources = applyCheckpointedResources(
info.resources(),
newCheckpointedResources);
CHECK_SOME(_totalResources)
<< "Failed to apply checkpointed resources "
<< newCheckpointedResources << " to agent's resources "
<< info.resources();
totalResources = _totalResources.get();
```
Setting agent resources to `_totalResources.get()` will break the agent resource accounting (if resource provider has resources). We might need to introduce a parameter to `checkpointResources` to not update `totalResources`.
src/tests/persistent_volume_tests.cpp
Lines 94 (patched)
<https://reviews.apache.org/r/63751/#comment268707>
Can you add a comment about what that `bool` represents? Ditto in ReservationTest
src/tests/persistent_volume_tests.cpp
Line 355 (original)
<https://reviews.apache.org/r/63751/#comment268712>
Hum, i don't expect you removing those checks. Just checking the offer is not sufficient because we speculate.
We should figure out a way to test that the reservation or persistent volume has actually been checkpointed.
One way is to add a helper in the fixture:
```
struct OperationMessage
{
// The resources in the message.
// 1) For CheckpointResourcesMessage, it's the checkpointed resources.
// 2) For ApplyOfferOperationMessage, it's the resources in the operation.
Resources resources;
};
Future<OperationMessage> message1 = getOperationMessage(slave.get()->pid);
...
AWAIT_READY(message1);
EXPECT_TRUE(message1.resources.contains(volume1));
AWAIT_READY(message2);
EXPECT_TRUE(message2.resources.contains(volume2));
```
Ditto elsewhere.
- Jie Yu
On Nov. 15, 2017, 4:06 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 15, 2017, 4:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
> src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
> src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
> src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 15, 2017, 5:06 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Parameterized reservation and persistent volume tests.
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
src/tests/persistent_volume_tests.cpp acfeac16884b00581a3523607ff26f44f6dca53a
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/4/
Changes: https://reviews.apache.org/r/63751/diff/3-4/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/
-----------------------------------------------------------
(Updated Nov. 15, 2017, 12:34 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed issues, added a helper to create status updates.
Bugs: MESOS-8211
https://issues.apache.org/jira/browse/MESOS-8211
Repository: mesos
Description
-------
Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.
Diffs (updated)
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
Diff: https://reviews.apache.org/r/63751/diff/3/
Changes: https://reviews.apache.org/r/63751/diff/2-3/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jie Yu <yu...@gmail.com>.
> On Nov. 14, 2017, 5:40 p.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Line 3720 (original), 3731 (patched)
> > <https://reviews.apache.org/r/63751/diff/2/?file=1890638#file1890638line3731>
> >
> > Let's add a CHECK here to test the operation is old operation.
> >
> > ```
> > CHECK(protobuf::isSpeculativeOperation(message.operation_info()));
> > ```
maybe just do
```
CHECK(message.operation_info().type() == Offer::Operation::RESERVE ||
message.operation_info().type() == Offer::Operation::UNRESERVE ||
message.operation_info().type() == Offer::Operation::CREATE ||
message.operation_info().type() == Offer::Operation::DESTROY);
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review190963
-----------------------------------------------------------
On Nov. 13, 2017, 2:21 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2017, 2:21 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 0ca4c6d689bf7d2c477174b039432ed7b6d0b650
> src/common/protobuf_utils.cpp 5739a63f8d87923c034375b88c4f0b3b19f4b521
> src/master/master.cpp 49dbaa979d692061a7b479b1db4511e8357b8baf
> src/slave/slave.cpp 7cb6661b55fb5437a1ffc447f974076aadd1eced
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for
agent local resources.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63751/#review190963
-----------------------------------------------------------
src/slave/slave.cpp
Line 3720 (original), 3731 (patched)
<https://reviews.apache.org/r/63751/#comment268560>
Let's add a CHECK here to test the operation is old operation.
```
CHECK(protobuf::isSpeculativeOperation(message.operation_info()));
```
src/slave/slave.cpp
Line 3726 (original), 3737 (patched)
<https://reviews.apache.org/r/63751/#comment268556>
insert a line above
src/slave/slave.cpp
Lines 3744-3745 (patched)
<https://reviews.apache.org/r/63751/#comment268545>
Use `protobuf::createOfferOperationStatus`?
src/slave/slave.cpp
Lines 3747-3754 (patched)
<https://reviews.apache.org/r/63751/#comment268546>
Can you introduce a `protobuf::createOfferOperationStatusUpdate` helper?
src/tests/reservation_tests.cpp
Lines 2300-2303 (patched)
<https://reviews.apache.org/r/63751/#comment268559>
I feel that a better way to test this is to parameterize the existing `ReservationTest` to handle agents w/ or w/o `RESOURCE_PROVIDER` capabilities.
You might need to adjust some tests that explicitly test `CheckpointResourcesMessage`. I feel that `SendingCheckpointResourcesMessage` is not necessary, we can probably just remove that test.
We probably should do the same for `PersistentVolumeTest`
For those test that wait on `FUTURE_PROTOBUF(CheckpointResourcesMessage, ...)`, we can probably change those to use `FUTURE_DISPATCH`
- Jie Yu
On Nov. 13, 2017, 2:21 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2017, 2:21 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 0ca4c6d689bf7d2c477174b039432ed7b6d0b650
> src/common/protobuf_utils.cpp 5739a63f8d87923c034375b88c4f0b3b19f4b521
> src/master/master.cpp 49dbaa979d692061a7b479b1db4511e8357b8baf
> src/slave/slave.cpp 7cb6661b55fb5437a1ffc447f974076aadd1eced
> src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780
>
>
> Diff: https://reviews.apache.org/r/63751/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>