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/11/14 14:36:14 UTC
Review Request 69337: Garbage collected disappeared resource
providers from master state.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Bugs: MESOS-9384
https://issues.apache.org/jira/browse/MESOS-9384
Repository: mesos
Description
-------
The master previously kept information on resource providers
indefinitely. This was confusing to API users who saw resource
providers reported which where not present anymore, and also made it
harder to derive actual cluster state.
With this patch we remove resource providers not reported by the agent
from master state. We still need to update the agent to not report
removed resource providers in a follow-up patch.
Diffs
-----
src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40
Diff: https://reviews.apache.org/r/69337/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 7, 2018, 3:18 a.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 8228 (patched)
> > <https://reviews.apache.org/r/69337/diff/1/?file=2107897#file2107897line8228>
> >
> > It seems to me that we should remove the disappeared resources even if there is no resource provider reported, otherwise if the operator manually deleted the resource provider configs, we will see the stale resources forever.
We need to branch here as `UpdateSlaveMessage` has overloaded meanings. We only want to update resource provider information if the agent sent us resource provider information (the agent ensures to it sets this field -- possible to an empty set -- whenever it send RP information); wouldn't want to trigger RP GC for updates to oversubscribed resources.
> On Dec. 7, 2018, 3:18 a.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 8260 (patched)
> > <https://reviews.apache.org/r/69337/diff/1/?file=2107897#file2107897line8260>
> >
> > Can you explain why we need to use this `resourceProvider` temporary variable here, given that `total_resources` is empty?
You are right, this should not be here.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/#review211104
-----------------------------------------------------------
On Dec. 13, 2018, 2:51 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69337/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2018, 2:51 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9384
> https://issues.apache.org/jira/browse/MESOS-9384
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master previously kept information on resource providers
> indefinitely. This was confusing to API users who saw resource
> providers reported which where not present anymore, and also made it
> harder to derive actual cluster state.
>
> With this patch we remove resource providers not reported by the agent
> from master state. We still need to update the agent to not report
> removed resource providers in a follow-up patch.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
>
>
> Diff: https://reviews.apache.org/r/69337/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/#review211104
-----------------------------------------------------------
src/master/master.cpp
Lines 8227 (patched)
<https://reviews.apache.org/r/69337/#comment296031>
Should this be called "garbage collection" or just "removal"?
src/master/master.cpp
Lines 8228 (patched)
<https://reviews.apache.org/r/69337/#comment296032>
It seems to me that we should remove the disappeared resources even if there is no resource provider reported, otherwise if the operator manually deleted the resource provider configs, we will see the stale resources forever.
src/master/master.cpp
Lines 8260 (patched)
<https://reviews.apache.org/r/69337/#comment296033>
Can you explain why we need to use this `resourceProvider` temporary variable here, given that `total_resources` is empty?
src/master/master.cpp
Lines 8262 (patched)
<https://reviews.apache.org/r/69337/#comment296034>
Not sure if we have discussed about this before. What's the pros and cons of totally erasing the resource provider, versus just removing its resources and operations?
- Chun-Hung Hsiao
On Nov. 14, 2018, 2:36 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69337/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2018, 2:36 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9384
> https://issues.apache.org/jira/browse/MESOS-9384
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master previously kept information on resource providers
> indefinitely. This was confusing to API users who saw resource
> providers reported which where not present anymore, and also made it
> harder to derive actual cluster state.
>
> With this patch we remove resource providers not reported by the agent
> from master state. We still need to update the agent to not report
> removed resource providers in a follow-up patch.
>
>
> Diffs
> -----
>
> src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40
>
>
> Diff: https://reviews.apache.org/r/69337/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Dec. 14, 2018, 7:07 p.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 8208 (patched)
> > <https://reviews.apache.org/r/69337/diff/3/?file=2114080#file2114080line8208>
> >
> > Does it make sense to move this line before L8016
>
> Benjamin Bannier wrote:
> I am not sure what you have in mind.
>
> It seems that performing this removal earlier makes the code look scarier than it is -- we'd e.g., expect readers to understand that in the current loop around l.8016, we do not touch RPs which hadn't been removed in some GC step.
>
> Please clarify or drop.
Since we state that we only update the resource providers if the message has the `resource_providers` field set, it seems natural if we reflect this in the implementation, i.e.,:
```
if (message.has_resourec_providers()) {
// Update received providers.
foreach (...) {
...
}
// GC disappeared providers.
foreach (...) {
...
}
}
```
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/#review211339
-----------------------------------------------------------
On Dec. 14, 2018, 3:56 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69337/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2018, 3:56 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9384
> https://issues.apache.org/jira/browse/MESOS-9384
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master previously kept information on resource providers
> indefinitely. This was confusing to API users who saw resource
> providers reported which where not present anymore, and also made it
> harder to derive actual cluster state.
>
> With this patch we remove resource providers not reported by the agent
> from master state. We still need to update the agent to not report
> removed resource providers in a follow-up patch.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69337/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 14, 2018, 8:07 p.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 8208 (patched)
> > <https://reviews.apache.org/r/69337/diff/3/?file=2114080#file2114080line8208>
> >
> > Does it make sense to move this line before L8016
I am not sure what you have in mind.
It seems that performing this removal earlier makes the code look scarier than it is -- we'd e.g., expect readers to understand that in the current loop around l.8016, we do not touch RPs which hadn't been removed in some GC step.
Please clarify or drop.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/#review211339
-----------------------------------------------------------
On Dec. 14, 2018, 4:56 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69337/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2018, 4:56 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9384
> https://issues.apache.org/jira/browse/MESOS-9384
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master previously kept information on resource providers
> indefinitely. This was confusing to API users who saw resource
> providers reported which where not present anymore, and also made it
> harder to derive actual cluster state.
>
> With this patch we remove resource providers not reported by the agent
> from master state. We still need to update the agent to not report
> removed resource providers in a follow-up patch.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69337/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/#review211339
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Lines 8208 (patched)
<https://reviews.apache.org/r/69337/#comment296324>
Does it make sense to move this line before L8016
- Chun-Hung Hsiao
On Dec. 14, 2018, 3:56 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69337/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2018, 3:56 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9384
> https://issues.apache.org/jira/browse/MESOS-9384
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master previously kept information on resource providers
> indefinitely. This was confusing to API users who saw resource
> providers reported which where not present anymore, and also made it
> harder to derive actual cluster state.
>
> With this patch we remove resource providers not reported by the agent
> from master state. We still need to update the agent to not report
> removed resource providers in a follow-up patch.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69337/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/
-----------------------------------------------------------
(Updated Jan. 10, 2019, 11:29 a.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Consolidated RP GC in block only executed in RP info is received.
Bugs: MESOS-9384
https://issues.apache.org/jira/browse/MESOS-9384
Repository: mesos
Description
-------
The master previously kept information on resource providers
indefinitely. This was confusing to API users who saw resource
providers reported which where not present anymore, and also made it
harder to derive actual cluster state.
With this patch we remove resource providers not reported by the agent
from master state. We still need to update the agent to not report
removed resource providers in a follow-up patch.
Diffs (updated)
-----
src/master/master.cpp 49b6e5c7d257bd9304215d47da13c9406c723cd8
src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
Diff: https://reviews.apache.org/r/69337/diff/4/
Changes: https://reviews.apache.org/r/69337/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/
-----------------------------------------------------------
(Updated Dec. 14, 2018, 4:56 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Bugs: MESOS-9384
https://issues.apache.org/jira/browse/MESOS-9384
Repository: mesos
Description
-------
The master previously kept information on resource providers
indefinitely. This was confusing to API users who saw resource
providers reported which where not present anymore, and also made it
harder to derive actual cluster state.
With this patch we remove resource providers not reported by the agent
from master state. We still need to update the agent to not report
removed resource providers in a follow-up patch.
Diffs (updated)
-----
src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
Diff: https://reviews.apache.org/r/69337/diff/3/
Changes: https://reviews.apache.org/r/69337/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/#review211313
-----------------------------------------------------------
src/master/master.cpp
Lines 8205 (patched)
<https://reviews.apache.org/r/69337/#comment296301>
How about making this if-statement include the RP update logic above, calculating the set of received resource providers in the foreach loop, and put the GC logic after that loop, so the whole update looks more cohesive?
src/master/master.cpp
Lines 8225-8230 (patched)
<https://reviews.apache.org/r/69337/#comment296302>
Can we simply do the following, i.e., is `slave->totalResources` always in sync with `slave->resourcesProviders.at(resourcePRoviderId).totalResources`?
```
slave->totalResources -= slave->resourceProviders.at(resourceProviderId).totalResources;
```
- Chun-Hung Hsiao
On Dec. 13, 2018, 1:51 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69337/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2018, 1:51 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9384
> https://issues.apache.org/jira/browse/MESOS-9384
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master previously kept information on resource providers
> indefinitely. This was confusing to API users who saw resource
> providers reported which where not present anymore, and also made it
> harder to derive actual cluster state.
>
> With this patch we remove resource providers not reported by the agent
> from master state. We still need to update the agent to not report
> removed resource providers in a follow-up patch.
>
>
> Diffs
> -----
>
> src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
>
>
> Diff: https://reviews.apache.org/r/69337/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69337: Garbage collected disappeared resource
providers from master state.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69337/
-----------------------------------------------------------
(Updated Dec. 13, 2018, 2:51 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Bugs: MESOS-9384
https://issues.apache.org/jira/browse/MESOS-9384
Repository: mesos
Description
-------
The master previously kept information on resource providers
indefinitely. This was confusing to API users who saw resource
providers reported which where not present anymore, and also made it
harder to derive actual cluster state.
With this patch we remove resource providers not reported by the agent
from master state. We still need to update the agent to not report
removed resource providers in a follow-up patch.
Diffs (updated)
-----
src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c
Diff: https://reviews.apache.org/r/69337/diff/2/
Changes: https://reviews.apache.org/r/69337/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier