You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@apache.org> on 2020/09/09 16:15:52 UTC
Review Request 72851: Added an `/offer_constraints_debug` endpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-10177
https://issues.apache.org/jira/browse/MESOS-10177
Repository: mesos
Description
-------
Added an `/offer_constraints_debug` endpoint.
Diffs
-----
src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
Diff: https://reviews.apache.org/r/72851/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/#review221859
-----------------------------------------------------------
Patch looks great!
Reviews applied: [72843, 72839, 72840, 72841, 72842, 72851]
Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh
- Mesos Reviewbot
On Sept. 9, 2020, 7:22 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72851/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 7:22 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an endpoint for debugging offer constraints.
>
>
> Diffs
> -----
>
> src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
> src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
> src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
> src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
>
>
> Diff: https://reviews.apache.org/r/72851/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/#review221840
-----------------------------------------------------------
Patch looks great!
Reviews applied: [72838, 72839, 72840, 72841, 72842, 72851]
Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh
- Mesos Reviewbot
On Sept. 9, 2020, 7:22 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72851/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 7:22 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an endpoint for debugging offer constraints.
>
>
> Diffs
> -----
>
> src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
> src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
> src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
> src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
>
>
> Diff: https://reviews.apache.org/r/72851/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Andrei Sekretenko <as...@apache.org>.
> On Sept. 15, 2020, 9:25 p.m., Benjamin Mahler wrote:
> > Sorry for the delay! Two things came up as I was thinking through this one:
> >
> > (1) I was expecting this to be an /allocator endpoint served from the allocator process rather than a /master endpoint served from the master process. The only advantage of being served by the master process is that it can leverage the parallel serving logic (any others?). The major downside is that we have to put constraint evaluation code in the master related logic, although since it's abstracted well it's not currently a big deal (much like we did with the /roles information).
> >
> > The main worry about it being on /master for me, is that if we need to add some more information in the endpoint that needs to come from the allocator then it will be better placed in the allocator. The obvious example coming to mind is: we're going to add resource quantity constraints that make the way agents get excluded much more dynamic than the attribute constraints alone. Do we want the master evaluating those? A more difficult example is per-constraint metrics around evaluations / pass/fail results / latency, etc, those might require the master to hold a shared copy of the overall filtering object if we were to serve it from /master?
> >
> > (2) One thing I realized while thinking about future constraints: we may want to split apart the exclusion lists as we add more types of constraints. E.g. "excluded", "excluded_by_attribute_constraints", "excluded_by_resource_constraints", etc? Otherwise, it might be pretty dynamic and hard to understand from the endpoint why the agent is overall being excluded?
> >
> > Thoughts?
The extendability concerns are perfectly legitimate.
Especially if we will want to report some detailed statistics at some point in the future.
The parallel serving logic is not that important, as we do not expect this endpoint to be called frequently. Sure, it would be nice to avoid stalling allocator while collecting the debug information, but it doesn't seem that critical.
My main concerns about moving the whole implementation into the allocator are **authentication** and **authorization**.
Two options here:
1) Perform initial handling of the request in the master and extend the allocator interface with a method returning the debug information (like `Future<JSON::Object> getOfferConstraintsDebugInfo(std::vector<FrameworkID>&&)` or, maybe, returning a protobuf `Future<allocator::OfferConstraintsDebugInfo> getOfferConstraintsDebugInfo(std::vector<FrameworkID>&&)`)
2) Extend allocator options with the readonly authentication domain (for which an authenticator is already set up by the master) and the authorizer; place the whole implementation into the Mesos allocator.
The slight drawback is that this way users of the authorizer interface proliferate (we are adding one more); IMO, this is not a big deal and probably even quite logical.
Probably we should indeed prefer the second option:
- this way, we don't add ad-hoc methods that the allocator would be obliged to implement
- we keeps the debug endpoint as such separate from the allocator interface
- providing an authentication realm and an authorizer to an allocator facilitates adding other authenticated authorizable HTTP endpoints to the allocator, should we ever need this in the future
What do you think?
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/#review221861
-----------------------------------------------------------
On Sept. 9, 2020, 9:22 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72851/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 9:22 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an endpoint for debugging offer constraints.
>
>
> Diffs
> -----
>
> src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
> src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
> src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
> src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
>
>
> Diff: https://reviews.apache.org/r/72851/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Sept. 15, 2020, 7:25 p.m., Benjamin Mahler wrote:
> > Sorry for the delay! Two things came up as I was thinking through this one:
> >
> > (1) I was expecting this to be an /allocator endpoint served from the allocator process rather than a /master endpoint served from the master process. The only advantage of being served by the master process is that it can leverage the parallel serving logic (any others?). The major downside is that we have to put constraint evaluation code in the master related logic, although since it's abstracted well it's not currently a big deal (much like we did with the /roles information).
> >
> > The main worry about it being on /master for me, is that if we need to add some more information in the endpoint that needs to come from the allocator then it will be better placed in the allocator. The obvious example coming to mind is: we're going to add resource quantity constraints that make the way agents get excluded much more dynamic than the attribute constraints alone. Do we want the master evaluating those? A more difficult example is per-constraint metrics around evaluations / pass/fail results / latency, etc, those might require the master to hold a shared copy of the overall filtering object if we were to serve it from /master?
> >
> > (2) One thing I realized while thinking about future constraints: we may want to split apart the exclusion lists as we add more types of constraints. E.g. "excluded", "excluded_by_attribute_constraints", "excluded_by_resource_constraints", etc? Otherwise, it might be pretty dynamic and hard to understand from the endpoint why the agent is overall being excluded?
> >
> > Thoughts?
>
> Andrei Sekretenko wrote:
> The extendability concerns are perfectly legitimate.
> Especially if we will want to report some detailed statistics at some point in the future.
>
> The parallel serving logic is not that important, as we do not expect this endpoint to be called frequently. Sure, it would be nice to avoid stalling allocator while collecting the debug information, but it doesn't seem that critical.
>
> My main concerns about moving the whole implementation into the allocator are **authentication** and **authorization**.
>
> Two options here:
> 1) Perform initial handling of the request in the master and extend the allocator interface with a method returning the debug information (like `Future<JSON::Object> getOfferConstraintsDebugInfo(std::vector<FrameworkID>&&)` or, maybe, returning a protobuf `Future<allocator::OfferConstraintsDebugInfo> getOfferConstraintsDebugInfo(std::vector<FrameworkID>&&)`)
>
> 2) Extend allocator options with the readonly authentication domain (for which an authenticator is already set up by the master) and the authorizer; place the whole implementation into the Mesos allocator.
> The slight drawback is that this way users of the authorizer interface proliferate (we are adding one more); IMO, this is not a big deal and probably even quite logical.
>
> Probably we should indeed prefer the second option:
> - this way, we don't add ad-hoc methods that the allocator would be obliged to implement
> - we keeps the debug endpoint as such separate from the allocator interface
> - providing an authentication realm and an authorizer to an allocator facilitates adding other authenticated authorizable HTTP endpoints to the allocator, should we ever need this in the future
>
> What do you think?
With 1) I think we'd probably have to go directly to string, for the same performance reasons that motivated us to move away from constructing / destructing intermediate objects with tons of subobjects.
2) sounds reasonable to me
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/#review221861
-----------------------------------------------------------
On Sept. 9, 2020, 7:22 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72851/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 7:22 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an endpoint for debugging offer constraints.
>
>
> Diffs
> -----
>
> src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
> src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
> src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
> src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
>
>
> Diff: https://reviews.apache.org/r/72851/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/#review221861
-----------------------------------------------------------
Sorry for the delay! Two things came up as I was thinking through this one:
(1) I was expecting this to be an /allocator endpoint served from the allocator process rather than a /master endpoint served from the master process. The only advantage of being served by the master process is that it can leverage the parallel serving logic (any others?). The major downside is that we have to put constraint evaluation code in the master related logic, although since it's abstracted well it's not currently a big deal (much like we did with the /roles information).
The main worry about it being on /master for me, is that if we need to add some more information in the endpoint that needs to come from the allocator then it will be better placed in the allocator. The obvious example coming to mind is: we're going to add resource quantity constraints that make the way agents get excluded much more dynamic than the attribute constraints alone. Do we want the master evaluating those? A more difficult example is per-constraint metrics around evaluations / pass/fail results / latency, etc, those might require the master to hold a shared copy of the overall filtering object if we were to serve it from /master?
(2) One thing I realized while thinking about future constraints: we may want to split apart the exclusion lists as we add more types of constraints. E.g. "excluded", "excluded_by_attribute_constraints", "excluded_by_resource_constraints", etc? Otherwise, it might be pretty dynamic and hard to understand from the endpoint why the agent is overall being excluded?
Thoughts?
- Benjamin Mahler
On Sept. 9, 2020, 7:22 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72851/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 7:22 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an endpoint for debugging offer constraints.
>
>
> Diffs
> -----
>
> src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
> src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
> src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
> src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
>
>
> Diff: https://reviews.apache.org/r/72851/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/#review221896
-----------------------------------------------------------
Ship it!
src/master/allocator/mesos/hierarchical.cpp
Lines 3246 (patched)
<https://reviews.apache.org/r/72851/#comment310954>
excluded_by_attribute_constraints is what the code is doing?
I was going to ask what the strategy for more constraints was, but I guess with the attribute constraint we can addd other exclusion keys for resource constraints etc
src/master/allocator/mesos/hierarchical.cpp
Lines 3247 (patched)
<https://reviews.apache.org/r/72851/#comment310953>
How about s/foo/role1/ and s/bar/role2/
src/master/allocator/mesos/hierarchical.cpp
Lines 3309-3311 (patched)
<https://reviews.apache.org/r/72851/#comment310955>
Looks good: easy to tell in the json which ones don't have constraints set.
- Benjamin Mahler
On Sept. 18, 2020, 4:56 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72851/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2020, 4:56 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an endpoint for debugging offer constraints.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp c6fca65b5b886dec26807871b750c6a9587faee0
> src/master/allocator/mesos/hierarchical.hpp 7e1980ef74bb4138b83707de7698f1993cc21e41
> src/master/allocator/mesos/hierarchical.cpp 47f8a2bfa267d68d39d2c255c7c97c8f213d5f69
> src/master/master.cpp 54b24b4eb8824737b10b3b4d6d51f387360570a1
>
>
> Diff: https://reviews.apache.org/r/72851/diff/3/
>
>
> Testing
> -------
>
> Tested manually
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Andrei Sekretenko <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/
-----------------------------------------------------------
(Updated Sept. 18, 2020, 6:56 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Switched to serving from allocator.
Bugs: MESOS-10177
https://issues.apache.org/jira/browse/MESOS-10177
Repository: mesos
Description
-------
Added an endpoint for debugging offer constraints.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp c6fca65b5b886dec26807871b750c6a9587faee0
src/master/allocator/mesos/hierarchical.hpp 7e1980ef74bb4138b83707de7698f1993cc21e41
src/master/allocator/mesos/hierarchical.cpp 47f8a2bfa267d68d39d2c255c7c97c8f213d5f69
src/master/master.cpp 54b24b4eb8824737b10b3b4d6d51f387360570a1
Diff: https://reviews.apache.org/r/72851/diff/3/
Changes: https://reviews.apache.org/r/72851/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72851: Added an endpoint for debugging offer
constraints.
Posted by Andrei Sekretenko <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72851/
-----------------------------------------------------------
(Updated Sept. 9, 2020, 9:22 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Replaced agent filtration state with excluded agent ID lists.
Summary (updated)
-----------------
Added an endpoint for debugging offer constraints.
Bugs: MESOS-10177
https://issues.apache.org/jira/browse/MESOS-10177
Repository: mesos
Description (updated)
-------
Added an endpoint for debugging offer constraints.
Diffs (updated)
-----
src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601
src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e
src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
Diff: https://reviews.apache.org/r/72851/diff/2/
Changes: https://reviews.apache.org/r/72851/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko