You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2018/02/09 17:27:41 UTC
Review Request 65591: Explicitly tracked resource providers in master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jan Schlicht.
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs
-----
src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94
Diff: https://reviews.apache.org/r/65591/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review197222
-----------------------------------------------------------
Patch looks great!
Reviews applied: [65587, 65588, 65589, 65590, 65591]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 9, 2018, 5:27 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2018, 5:27 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
> src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
> src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94
>
>
> Diff: https://reviews.apache.org/r/65591/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review197175
-----------------------------------------------------------
PASS: Mesos patch 65591 was successfully built and tested.
Reviews applied: `['65587', '65588', '65589', '65590', '65591']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65591
- Mesos Reviewbot Windows
On Feb. 9, 2018, 7:27 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2018, 7:27 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
> src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
> src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94
>
>
> Diff: https://reviews.apache.org/r/65591/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 7324 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7326>
> >
> > Is there any case where `message.has_resource_version_uuid() && slave->resourceVersion.isNone()` will be true?
> >
> > I'm wondering if we should do something like:
> > ```
> > if (!updated && message.has_resource_version_uuid()) {
> > CHECK_SOME(slave->resourceVersion);
> > if (message.resource_version_uuid() != slave->resourceVersion.get()) {
> > updated = true;
> > }
> > }
> > ```
It is possible on the proto level since e.g., `RegisterSlaveMessage.resource_version_uuid` is `optional`. I'd argue that the currently proposed implementation is more conservative in that it would run the full handler even in the case you point out instead of aborting.
I'd suggest we keep the impl like it is currently. Dropping.
> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 7722 (original), 7680-7681 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7847>
> >
> > Perhaps we also want to rescind offers when `message.has_resource_providers()` is false? In that case, the agent has told us that it has no registered RPs, so offers with RP resources would not be valid?
I do not think that is true. Only when the `resource_providers` field is set does the agent send any resource provider information. The field could contain information on zero or more resource providers in its `providers` field, but we do not distinguish that here, exactly as you suggest.
Dropping.
> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 10572-10574 (original), 10534-10536 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line10701>
> >
> > Are you sure this will always be true? Could a framework send us a well-formed, but unknown RP ID in a message and make this false?
This is an important invariant of the current implementation, see https://issues.apache.org/jira/browse/MESOS-8321.
Dropping.
> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 11515 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line11686>
> >
> > Ditto with this CHECK - are we sure that a well-formed but unknown RP ID wouldn't hit this?
Ditto, see https://issues.apache.org/jira/browse/MESOS-8321.
Dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review198741
-----------------------------------------------------------
On March 7, 2018, 12:36 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated March 7, 2018, 12:36 p.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
> src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
> src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb
> src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35
>
>
> Diff: https://reviews.apache.org/r/65591/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 7, 2018, 3:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 7324 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7326>
> >
> > Is there any case where `message.has_resource_version_uuid() && slave->resourceVersion.isNone()` will be true?
> >
> > I'm wondering if we should do something like:
> > ```
> > if (!updated && message.has_resource_version_uuid()) {
> > CHECK_SOME(slave->resourceVersion);
> > if (message.resource_version_uuid() != slave->resourceVersion.get()) {
> > updated = true;
> > }
> > }
> > ```
>
> Benjamin Bannier wrote:
> It is possible on the proto level since e.g., `RegisterSlaveMessage.resource_version_uuid` is `optional`. I'd argue that the currently proposed implementation is more conservative in that it would run the full handler even in the case you point out instead of aborting.
>
> I'd suggest we keep the impl like it is currently. Dropping.
What does it mean if `message.has_resource_version_uuid() && slave->resourceVersion.isNone()` is true? IIUC, that means that the agent previously registered with no resource version UUID for agent default resources, and it is now providing a resource version UUID in an UpdateSlaveMessage. Doesn't this indicate an error in the agent? Does it make sense to run the rest of the handler in such a case? I'm not sure what kind of precedent we have in the code for aborting the master on unexpected input from an agent, but if that's something we're in the habit of doing, this would seem like a suitable place for it.
> On March 7, 2018, 3:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 7722 (original), 7680-7681 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7847>
> >
> > Perhaps we also want to rescind offers when `message.has_resource_providers()` is false? In that case, the agent has told us that it has no registered RPs, so offers with RP resources would not be valid?
>
> Benjamin Bannier wrote:
> I do not think that is true. Only when the `resource_providers` field is set does the agent send any resource provider information. The field could contain information on zero or more resource providers in its `providers` field, but we do not distinguish that here, exactly as you suggest.
>
> Dropping.
IIUC, the agent always sends info on all registered RPs in each UpdateSlaveMessage. So, if the master receives an UpdateSlaveMessage which indicates that no RPs are currently registered on an agent, doesn't that mean that offers for that agent with RP resources are currently invalid?
> On March 7, 2018, 3:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 10572-10574 (original), 10534-10536 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line10701>
> >
> > Are you sure this will always be true? Could a framework send us a well-formed, but unknown RP ID in a message and make this false?
>
> Benjamin Bannier wrote:
> This is an important invariant of the current implementation, see https://issues.apache.org/jira/browse/MESOS-8321.
>
> Dropping.
Are we sure that invariant holds in this case? The resource provider ID here is framework-supplied, and I don't think we validate that it's present in the master state before we hit this code.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review198741
-----------------------------------------------------------
On March 7, 2018, 11:36 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated March 7, 2018, 11:36 a.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
> src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
> src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb
> src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35
>
>
> Diff: https://reviews.apache.org/r/65591/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 7722 (original), 7680-7681 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7847>
> >
> > Perhaps we also want to rescind offers when `message.has_resource_providers()` is false? In that case, the agent has told us that it has no registered RPs, so offers with RP resources would not be valid?
>
> Benjamin Bannier wrote:
> I do not think that is true. Only when the `resource_providers` field is set does the agent send any resource provider information. The field could contain information on zero or more resource providers in its `providers` field, but we do not distinguish that here, exactly as you suggest.
>
> Dropping.
>
> Greg Mann wrote:
> IIUC, the agent always sends info on all registered RPs in each UpdateSlaveMessage. So, if the master receives an UpdateSlaveMessage which indicates that no RPs are currently registered on an agent, doesn't that mean that offers for that agent with RP resources are currently invalid?
Yes, the offers will be rescinded below.
> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 10572-10574 (original), 10534-10536 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line10701>
> >
> > Are you sure this will always be true? Could a framework send us a well-formed, but unknown RP ID in a message and make this false?
>
> Benjamin Bannier wrote:
> This is an important invariant of the current implementation, see https://issues.apache.org/jira/browse/MESOS-8321.
>
> Dropping.
>
> Greg Mann wrote:
> Are we sure that invariant holds in this case? The resource provider ID here is framework-supplied, and I don't think we validate that it's present in the master state before we hit this code.
On the accept path the master will verify that (i) it knows about the offer (https://github.com/apache/mesos/blob/843e5e85939d848b0898753c9d7542ecc997135c/src/master/master.cpp#L3906-L3924), and (ii) that the operations are consistent with the originally offered resources (https://github.com/apache/mesos/blob/843e5e85939d848b0898753c9d7542ecc997135c/src/master/master.cpp#L4530-L4544).
For a resource provider to be unknown to the master here it would either have to be explicitly removed by the master, or be made up by the framework. In the first case the offer would be invalid, in the second the operation consistency check would fail.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review198741
-----------------------------------------------------------
On March 7, 2018, 12:36 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated March 7, 2018, 12:36 p.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
> src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
> src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb
> src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35
>
>
> Diff: https://reviews.apache.org/r/65591/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review198741
-----------------------------------------------------------
Thanks Benjamin - this is a nice improvement!
src/master/master.cpp
Lines 7324 (patched)
<https://reviews.apache.org/r/65591/#comment278967>
Is there any case where `message.has_resource_version_uuid() && slave->resourceVersion.isNone()` will be true?
I'm wondering if we should do something like:
```
if (!updated && message.has_resource_version_uuid()) {
CHECK_SOME(slave->resourceVersion);
if (message.resource_version_uuid() != slave->resourceVersion.get()) {
updated = true;
}
}
```
src/master/master.cpp
Lines 7324-7325 (original), 7361-7362 (patched)
<https://reviews.apache.org/r/65591/#comment278968>
Suggestion: perhaps using names like `receivedProvider` and `storedProvider` (or `oldProvider`/`newProvider`?) might improve readability.
So we would have checks like
```
if (receivedProvider.info != storedProvider.info() ...
```
src/master/master.cpp
Line 7492 (original), 7433 (patched)
<https://reviews.apache.org/r/65591/#comment278989>
Nit: newline after this comment, since it applies to multiple blocks of code below.
src/master/master.cpp
Lines 7525 (patched)
<https://reviews.apache.org/r/65591/#comment278990>
s/ways/ways of/
src/master/master.cpp
Lines 7530-7538 (patched)
<https://reviews.apache.org/r/65591/#comment278991>
`framework == nullptr` can represent one of two cases at this point:
1) The operation did not have a framework ID set
2) The framework specified by the operation does not have its info stored in the master
In both cases, I believe we want to continue. In only the second case do we want to log the warning.
src/master/master.cpp
Lines 7635-7640 (original), 7596-7599 (patched)
<https://reviews.apache.org/r/65591/#comment278992>
Nit: I think you could remove the newline here.
src/master/master.cpp
Line 7722 (original), 7680-7681 (patched)
<https://reviews.apache.org/r/65591/#comment279001>
Perhaps we also want to rescind offers when `message.has_resource_providers()` is false? In that case, the agent has told us that it has no registered RPs, so offers with RP resources would not be valid?
src/master/master.cpp
Lines 10572-10574 (original), 10534-10536 (patched)
<https://reviews.apache.org/r/65591/#comment279003>
Are you sure this will always be true? Could a framework send us a well-formed, but unknown RP ID in a message and make this false?
src/master/master.cpp
Lines 11515 (patched)
<https://reviews.apache.org/r/65591/#comment279004>
Ditto with this CHECK - are we sure that a well-formed but unknown RP ID wouldn't hit this?
- Greg Mann
On March 5, 2018, 10:21 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated March 5, 2018, 10:21 p.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
> src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
> src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df
> src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c
>
>
> Diff: https://reviews.apache.org/r/65591/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
(Updated March 14, 2018, 1:03 p.m.)
Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
Changes
-------
Rebased.
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs (updated)
-----
src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
src/master/http.cpp cf03d8bf7df583c849a3b2a326d3772820181b0f
src/master/master.hpp 8bf2c763dafdb7df55c46a56f2ff66f2a951d947
src/master/master.cpp 223ebf29ac4dd1dea9080e4bef4b2d4d064d847f
Diff: https://reviews.apache.org/r/65591/diff/9/
Changes: https://reviews.apache.org/r/65591/diff/8-9/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review199095
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On March 7, 2018, 11:36 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated March 7, 2018, 11:36 a.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
> src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
> src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb
> src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35
>
>
> Diff: https://reviews.apache.org/r/65591/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
(Updated March 7, 2018, 12:36 p.m.)
Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed issues raised by Greg.
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs (updated)
-----
src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb
src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35
Diff: https://reviews.apache.org/r/65591/diff/8/
Changes: https://reviews.apache.org/r/65591/diff/7-8/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
(Updated March 5, 2018, 11:21 p.m.)
Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
Changes
-------
* allowed shrinkinf of RPs as pointed out by Greg.
* set resource versions when reconciling known resource providers.
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs (updated)
-----
src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df
src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c
Diff: https://reviews.apache.org/r/65591/diff/7/
Changes: https://reviews.apache.org/r/65591/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
(Updated Feb. 23, 2018, 4:22 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
* fixed `GET_OPERATIONS` handler in master
* fixed lifetime when adding resource providers after master failover
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs (updated)
-----
src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad
src/master/master.hpp 6569c9ea0677f193ec56c276e6deea3f2fe86514
src/master/master.cpp e7d5ac67a94f927bb747bd283a3bf9671b2b8f44
Diff: https://reviews.apache.org/r/65591/diff/5/
Changes: https://reviews.apache.org/r/65591/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
(Updated Feb. 16, 2018, 3:12 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Fixed member documentation.
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs (updated)
-----
src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
src/master/master.hpp c4d3c8080dc7899968b1030696172ed73d473bc1
src/master/master.cpp b06d7a6e2fbbb81b97eaf537d5b6745c73dc867d
Diff: https://reviews.apache.org/r/65591/diff/3/
Changes: https://reviews.apache.org/r/65591/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 16, 2018, 2:13 a.m., Gaston Kleiman wrote:
> > src/master/master.cpp
> > Line 7307 (original), 7351 (patched)
> > <https://reviews.apache.org/r/65591/diff/2/?file=1961540#file1961540line7353>
> >
> > Do we plan to support resource providers with info (so not the agent-default resources), but without an ID?
> >
> > Line 7470 indicates that we don't.
Added `CHECK`s here as well.
> On Feb. 16, 2018, 2:13 a.m., Gaston Kleiman wrote:
> > src/master/master.cpp
> > Lines 7516 (patched)
> > <https://reviews.apache.org/r/65591/diff/2/?file=1961540#file1961540line7672>
> >
> > It doesnt look like `operation_` is used after this line. Can we revert to inlining the creation of the operation?
Like the comment above indicated this should have updated `operations`, fixed the code.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review197652
-----------------------------------------------------------
On Feb. 16, 2018, 3:12 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2018, 3:12 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
> src/master/master.hpp c4d3c8080dc7899968b1030696172ed73d473bc1
> src/master/master.cpp b06d7a6e2fbbb81b97eaf537d5b6745c73dc867d
>
>
> Diff: https://reviews.apache.org/r/65591/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review197652
-----------------------------------------------------------
src/master/master.hpp
Lines 279-290 (patched)
<https://reviews.apache.org/r/65591/#comment277878>
This looks strange.
You probably want to remove these lines.
src/master/master.cpp
Line 7307 (original), 7351 (patched)
<https://reviews.apache.org/r/65591/#comment277881>
Do we plan to support resource providers with info (so not the agent-default resources), but without an ID?
Line 7470 indicates that we don't.
src/master/master.cpp
Line 7488 (original), 7427-7428 (patched)
<https://reviews.apache.org/r/65591/#comment277883>
Fix indentation.
src/master/master.cpp
Lines 7516 (patched)
<https://reviews.apache.org/r/65591/#comment277888>
It doesnt look like `operation_` is used after this line. Can we revert to inlining the creation of the operation?
- Gaston Kleiman
On Feb. 15, 2018, 6:54 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2018, 6:54 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
> src/master/master.hpp c4d3c8080dc7899968b1030696172ed73d473bc1
> src/master/master.cpp b06d7a6e2fbbb81b97eaf537d5b6745c73dc867d
>
>
> Diff: https://reviews.apache.org/r/65591/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/
-----------------------------------------------------------
(Updated Feb. 15, 2018, 3:54 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
* addressed comments from Jan
* re-added reconciliation of operations on agent-default resources
Bugs: MESOS-8382
https://issues.apache.org/jira/browse/MESOS-8382
Repository: mesos
Description
-------
This patch adds explicit tracking of resource providers to the master
process. While we already had explicitly send resource provider
information in e.g., `UpdateSlaveMessage`, we only stored that
information aggregated over the full agent in the master up to now.
Diffs (updated)
-----
src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
src/master/master.hpp c4d3c8080dc7899968b1030696172ed73d473bc1
src/master/master.cpp b06d7a6e2fbbb81b97eaf537d5b6745c73dc867d
Diff: https://reviews.apache.org/r/65591/diff/2/
Changes: https://reviews.apache.org/r/65591/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 13, 2018, 5:30 p.m., Jan Schlicht wrote:
> > src/master/master.cpp
> > Lines 7334-7336 (patched)
> > <https://reviews.apache.org/r/65591/diff/1/?file=1955495#file1955495line7345>
> >
> > You compare operations here for resource providers. This isn't done in the code above for agent operations. Please add a comment, why this is necessary here but not for agent operations.
I added the equivalent check for operations on agent default resources.
> On Feb. 13, 2018, 5:30 p.m., Jan Schlicht wrote:
> > src/master/master.cpp
> > Lines 7448 (patched)
> > <https://reviews.apache.org/r/65591/diff/1/?file=1955495#file1955495line7621>
> >
> > This doesn't feel right. We don't want to add an operation to the agent here, only to the respective resource provider -- but might want to update used resources of the agent.
I have updated `addOperation`, `removeOperation` and `getOperation` to locally take resource providers into account. That way callers do not need to extract resource provider ids before being able work with operations (we have this information here though).
> On Feb. 13, 2018, 5:30 p.m., Jan Schlicht wrote:
> > src/master/master.cpp
> > Lines 7626-7627 (original), 7537-7538 (patched)
> > <https://reviews.apache.org/r/65591/diff/1/?file=1955495#file1955495line7723>
> >
> > Probably needs to be updated to handle resource provider operations.
Could you clarify your comment? `updateOperation` is a helper function which mutates the passed operation; AFAICT it is not intended to even know anything about where the operation is tracked.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review197410
-----------------------------------------------------------
On Feb. 15, 2018, 3:54 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2018, 3:54 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
> src/master/master.hpp c4d3c8080dc7899968b1030696172ed73d473bc1
> src/master/master.cpp b06d7a6e2fbbb81b97eaf537d5b6745c73dc867d
>
>
> Diff: https://reviews.apache.org/r/65591/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 65591: Explicitly tracked resource providers in
master.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review197410
-----------------------------------------------------------
src/master/master.cpp
Line 7295 (original), 7329 (patched)
<https://reviews.apache.org/r/65591/#comment277558>
This should be `!contains`, right?
src/master/master.cpp
Lines 7334-7336 (patched)
<https://reviews.apache.org/r/65591/#comment277560>
You compare operations here for resource providers. This isn't done in the code above for agent operations. Please add a comment, why this is necessary here but not for agent operations.
src/master/master.cpp
Line 7392 (original), 7351 (patched)
<https://reviews.apache.org/r/65591/#comment277562>
Name this `resourceProvider` to be consistent with the code above (or use `provider` in the code above). Here and below when iterating over `message.resource_providers().providers()`.
src/master/master.cpp
Lines 7546-7547 (original), 7437-7438 (patched)
<https://reviews.apache.org/r/65591/#comment277564>
This makes no sense, slave operations are tracked separately from resource provider operations. Please remove.
src/master/master.cpp
Lines 7448 (patched)
<https://reviews.apache.org/r/65591/#comment277565>
This doesn't feel right. We don't want to add an operation to the agent here, only to the respective resource provider -- but might want to update used resources of the agent.
src/master/master.cpp
Lines 7516 (patched)
<https://reviews.apache.org/r/65591/#comment277566>
This is redundant, as we're iterating over `oldProvider.operations` here.
src/master/master.cpp
Lines 7517-7518 (patched)
<https://reviews.apache.org/r/65591/#comment277567>
Could iterate using `foreachpair` instead.
src/master/master.cpp
Lines 7520 (patched)
<https://reviews.apache.org/r/65591/#comment277568>
Also redundant, as that guaranteed by the `else` case we're currently in.
src/master/master.cpp
Line 7616 (original), 7527 (patched)
<https://reviews.apache.org/r/65591/#comment277569>
Agent shouldn't keep track of resource provider operations.
src/master/master.cpp
Lines 7626-7627 (original), 7537-7538 (patched)
<https://reviews.apache.org/r/65591/#comment277570>
Probably needs to be updated to handle resource provider operations.
- Jan Schlicht
On Feb. 9, 2018, 6:27 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2018, 6:27 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f
> src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
> src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94
>
>
> Diff: https://reviews.apache.org/r/65591/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>