You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/07/02 19:57:21 UTC
Review Request 70996: Implemented master endpoints for agent draining.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70996/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Vinod Kone.
Bugs: MESOS-9814
https://issues.apache.org/jira/browse/MESOS-9814
Repository: mesos
Description
-------
This fleshes out three master calls:
* DRAIN_AGENT
* DEACTIVATE_AGENT
* REACTIVATE_AGENT
The master now stores a mapping of agent draining or deactivation
information. When an agent reconnects, this information is checked
before informing the allocator about the agent.
This commit leaves out certain aspects of each endpoint's validation,
such as checking if draining agents are not in maintenance schedules.
The DRAIN_AGENT call is not completely implemented here, because the
transition from DRAINING to DRAINED will be added in a separate commit.
Diffs
-----
src/master/http.cpp b42ebb953e0510e83ec6bd041cbddbeb8f60067c
src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
Diff: https://reviews.apache.org/r/70996/diff/1/
Testing
-------
TODO: At this point, there isn't anything exposed by the master which can be used to check the results of these APIs.
Thanks,
Joseph Wu
Re: Review Request 70996: Implemented master endpoints for agent
draining.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On July 8, 2019, 3 p.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 3879-3881 (patched)
> > <https://reviews.apache.org/r/70996/diff/1/?file=2153251#file2153251line3879>
> >
> > Nit: I think we usually place the `&&` at the end of the preceding line rather than the beginning of the next.
> >
> > Ditto below.
> >
> > Also, I wonder if we should factor this block out into a helper like `bool isKnownSlave(const SlaveID&)`? Then we have a single location to update if/when we add more containers of known slaves?
A helper might not be usable since we have nearly a dozen categories of agents within the master, and all of them are "known" but don't necessarily apply to every case. Like how unreachable agents only look at recovered/registered agents, or how gone agents are not part of any other set.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70996/#review216421
-----------------------------------------------------------
On July 2, 2019, 12:57 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70996/
> -----------------------------------------------------------
>
> (Updated July 2, 2019, 12:57 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-9814
> https://issues.apache.org/jira/browse/MESOS-9814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fleshes out three master calls:
> * DRAIN_AGENT
> * DEACTIVATE_AGENT
> * REACTIVATE_AGENT
>
> The master now stores a mapping of agent draining or deactivation
> information. When an agent reconnects, this information is checked
> before informing the allocator about the agent.
>
> This commit leaves out certain aspects of each endpoint's validation,
> such as checking if draining agents are not in maintenance schedules.
>
> The DRAIN_AGENT call is not completely implemented here, because the
> transition from DRAINING to DRAINED will be added in a separate commit.
>
>
> Diffs
> -----
>
> src/master/http.cpp b42ebb953e0510e83ec6bd041cbddbeb8f60067c
> src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
> src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
>
>
> Diff: https://reviews.apache.org/r/70996/diff/1/
>
>
> Testing
> -------
>
> TODO: At this point, there isn't anything exposed by the master which can be used to check the results of these APIs.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70996: Implemented master endpoints for agent
draining.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70996/#review216421
-----------------------------------------------------------
src/master/http.cpp
Lines 3879-3881 (patched)
<https://reviews.apache.org/r/70996/#comment303646>
Nit: I think we usually place the `&&` at the end of the preceding line rather than the beginning of the next.
Ditto below.
Also, I wonder if we should factor this block out into a helper like `bool isKnownSlave(const SlaveID&)`? Then we have a single location to update if/when we add more containers of known slaves?
src/master/master.hpp
Lines 678 (patched)
<https://reviews.apache.org/r/70996/#comment303650>
Provide a comment documenting this new function? It may not be consistent, but let's start setting a good example now :)
src/master/master.hpp
Lines 2109-2110 (patched)
<https://reviews.apache.org/r/70996/#comment303647>
s/both admitted and unreachable/admitted, unreachable, and recovered/
Ditto below.
src/master/master.cpp
Lines 3452 (patched)
<https://reviews.apache.org/r/70996/#comment303648>
Having this conditional within `Master::reactivate()` looks a bit weird to me. What do you think about moving it to the callsite in `Master::___reregisterSlave()`, and instead placing a `CHECK(!slaves.deactivated.contains(slave->id));` within this function?
- Greg Mann
On July 2, 2019, 7:57 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70996/
> -----------------------------------------------------------
>
> (Updated July 2, 2019, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-9814
> https://issues.apache.org/jira/browse/MESOS-9814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fleshes out three master calls:
> * DRAIN_AGENT
> * DEACTIVATE_AGENT
> * REACTIVATE_AGENT
>
> The master now stores a mapping of agent draining or deactivation
> information. When an agent reconnects, this information is checked
> before informing the allocator about the agent.
>
> This commit leaves out certain aspects of each endpoint's validation,
> such as checking if draining agents are not in maintenance schedules.
>
> The DRAIN_AGENT call is not completely implemented here, because the
> transition from DRAINING to DRAINED will be added in a separate commit.
>
>
> Diffs
> -----
>
> src/master/http.cpp b42ebb953e0510e83ec6bd041cbddbeb8f60067c
> src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
> src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
>
>
> Diff: https://reviews.apache.org/r/70996/diff/1/
>
>
> Testing
> -------
>
> TODO: At this point, there isn't anything exposed by the master which can be used to check the results of these APIs.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70996: Implemented master endpoints for agent
draining.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70996/#review216636
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On July 15, 2019, 11:30 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70996/
> -----------------------------------------------------------
>
> (Updated July 15, 2019, 11:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-9814
> https://issues.apache.org/jira/browse/MESOS-9814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fleshes out three master calls:
> * DRAIN_AGENT
> * DEACTIVATE_AGENT
> * REACTIVATE_AGENT
>
> The master now stores a mapping of agent draining or deactivation
> information. When an agent reconnects, this information is checked
> before informing the allocator about the agent.
>
> This commit leaves out certain aspects of each endpoint's validation,
> such as checking if draining agents are not in maintenance schedules.
>
> The DRAIN_AGENT call is not completely implemented here, because the
> transition from DRAINING to DRAINED will be added in a separate commit.
>
>
> Diffs
> -----
>
> src/master/http.cpp cd0f40cb7b966d6620e3fb49d4c08807185c9101
> src/master/master.hpp e8def83fe9bcee19772df9a9764852bc694c5247
> src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
>
>
> Diff: https://reviews.apache.org/r/70996/diff/3/
>
>
> Testing
> -------
>
> TODO: At this point, there isn't anything exposed by the master which can be used to check the results of these APIs.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70996: Implemented master endpoints for agent
draining.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70996/
-----------------------------------------------------------
(Updated July 15, 2019, 4:30 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Vinod Kone.
Changes
-------
Guarded against marking an agent gone while drying to mark it for draining.
Bugs: MESOS-9814
https://issues.apache.org/jira/browse/MESOS-9814
Repository: mesos
Description
-------
This fleshes out three master calls:
* DRAIN_AGENT
* DEACTIVATE_AGENT
* REACTIVATE_AGENT
The master now stores a mapping of agent draining or deactivation
information. When an agent reconnects, this information is checked
before informing the allocator about the agent.
This commit leaves out certain aspects of each endpoint's validation,
such as checking if draining agents are not in maintenance schedules.
The DRAIN_AGENT call is not completely implemented here, because the
transition from DRAINING to DRAINED will be added in a separate commit.
Diffs (updated)
-----
src/master/http.cpp cd0f40cb7b966d6620e3fb49d4c08807185c9101
src/master/master.hpp e8def83fe9bcee19772df9a9764852bc694c5247
src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
Diff: https://reviews.apache.org/r/70996/diff/3/
Changes: https://reviews.apache.org/r/70996/diff/2-3/
Testing
-------
TODO: At this point, there isn't anything exposed by the master which can be used to check the results of these APIs.
Thanks,
Joseph Wu
Re: Review Request 70996: Implemented master endpoints for agent
draining.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70996/
-----------------------------------------------------------
(Updated July 9, 2019, 12:27 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Vinod Kone.
Changes
-------
Added/modified some comments and style.
Changed `reactivate` to include a CHECK.
Bugs: MESOS-9814
https://issues.apache.org/jira/browse/MESOS-9814
Repository: mesos
Description
-------
This fleshes out three master calls:
* DRAIN_AGENT
* DEACTIVATE_AGENT
* REACTIVATE_AGENT
The master now stores a mapping of agent draining or deactivation
information. When an agent reconnects, this information is checked
before informing the allocator about the agent.
This commit leaves out certain aspects of each endpoint's validation,
such as checking if draining agents are not in maintenance schedules.
The DRAIN_AGENT call is not completely implemented here, because the
transition from DRAINING to DRAINED will be added in a separate commit.
Diffs (updated)
-----
src/master/http.cpp b42ebb953e0510e83ec6bd041cbddbeb8f60067c
src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
Diff: https://reviews.apache.org/r/70996/diff/2/
Changes: https://reviews.apache.org/r/70996/diff/1-2/
Testing
-------
TODO: At this point, there isn't anything exposed by the master which can be used to check the results of these APIs.
Thanks,
Joseph Wu