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 2017/09/06 12:00:40 UTC
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 6, 2017, 2 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04
src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
src/slave/slave.cpp df920ec07cd59c7ba6baccfc1c20ed3809f187d6
src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
Diff: https://reviews.apache.org/r/61183/diff/4/
Changes: https://reviews.apache.org/r/61183/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Sept. 7, 2017, 1:32 a.m., Jie Yu wrote:
> > src/slave/slave.hpp
> > Lines 658 (patched)
> > <https://reviews.apache.org/r/61183/diff/5/?file=1816402#file1816402line658>
> >
> > I am not sure if keeping another field just for resoruce provider provided resources in the agent is a good idea or not.
> >
> > I'd much prefer we keep a single `totalResources` for regular resources (both resource provider provided or not), and `oversubscribedResources` only for oversubsribed resources.
>
> Benjamin Bannier wrote:
> The advantage of using a dedicated member is that with it it is possible to detect whether any resources came from resource providers. With that we can avoid sending `UpdateSlaveMessage`s e.g., on reregistration when no resource providers registered (not sure how to do this without storing at least some data).
>
> I'd suggest introducing this member as it captures everything we need.
>
> Jie Yu wrote:
> I think we should move toward a world to have just one field tracking the total resources of the agent. With this patch, we'll have three: `totalResources`, `oversubscribedResources` and `resourceProviderResources`. It becomes very hard to read for someone who is not familiar with the code.
>
> Regarding your comment on optimizing. I think the right way is to track the last total sent to the master (i.e., `lastSyncedTotalResources`) and only send `UpdateSlaveMessage` if there is a diff.
>
> Jie Yu wrote:
> Another benefit is to be in sync with what we track in master. in master, we track `slave->totalResources`. I don't think we want to keep a separate `resourceProviderResources` in the master, right?
I adjusted the patch to track the last synced resources instead of explicitly tracking the resource provider resources.
I believe this requires updating `lastSyncedResources` in a few places where we currently trigger updates to the agents total.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review184751
-----------------------------------------------------------
On Sept. 8, 2017, 12:38 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 12:38 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
> src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
> src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
>
>
> Diff: https://reviews.apache.org/r/61183/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 6, 2017, 11:32 p.m., Jie Yu wrote:
> > src/slave/slave.hpp
> > Lines 658 (patched)
> > <https://reviews.apache.org/r/61183/diff/5/?file=1816402#file1816402line658>
> >
> > I am not sure if keeping another field just for resoruce provider provided resources in the agent is a good idea or not.
> >
> > I'd much prefer we keep a single `totalResources` for regular resources (both resource provider provided or not), and `oversubscribedResources` only for oversubsribed resources.
>
> Benjamin Bannier wrote:
> The advantage of using a dedicated member is that with it it is possible to detect whether any resources came from resource providers. With that we can avoid sending `UpdateSlaveMessage`s e.g., on reregistration when no resource providers registered (not sure how to do this without storing at least some data).
>
> I'd suggest introducing this member as it captures everything we need.
I think we should move toward a world to have just one field tracking the total resources of the agent. With this patch, we'll have three: `totalResources`, `oversubscribedResources` and `resourceProviderResources`. It becomes very hard to read for someone who is not familiar with the code.
Regarding your comment on optimizing. I think the right way is to track the last total sent to the master (i.e., `lastSyncedTotalResources`) and only send `UpdateSlaveMessage` if there is a diff.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review184751
-----------------------------------------------------------
On Sept. 7, 2017, 3:13 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 3:13 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
> src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
> src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
>
>
> Diff: https://reviews.apache.org/r/61183/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 6, 2017, 11:32 p.m., Jie Yu wrote:
> > src/slave/slave.hpp
> > Lines 658 (patched)
> > <https://reviews.apache.org/r/61183/diff/5/?file=1816402#file1816402line658>
> >
> > I am not sure if keeping another field just for resoruce provider provided resources in the agent is a good idea or not.
> >
> > I'd much prefer we keep a single `totalResources` for regular resources (both resource provider provided or not), and `oversubscribedResources` only for oversubsribed resources.
>
> Benjamin Bannier wrote:
> The advantage of using a dedicated member is that with it it is possible to detect whether any resources came from resource providers. With that we can avoid sending `UpdateSlaveMessage`s e.g., on reregistration when no resource providers registered (not sure how to do this without storing at least some data).
>
> I'd suggest introducing this member as it captures everything we need.
>
> Jie Yu wrote:
> I think we should move toward a world to have just one field tracking the total resources of the agent. With this patch, we'll have three: `totalResources`, `oversubscribedResources` and `resourceProviderResources`. It becomes very hard to read for someone who is not familiar with the code.
>
> Regarding your comment on optimizing. I think the right way is to track the last total sent to the master (i.e., `lastSyncedTotalResources`) and only send `UpdateSlaveMessage` if there is a diff.
Another benefit is to be in sync with what we track in master. in master, we track `slave->totalResources`. I don't think we want to keep a separate `resourceProviderResources` in the master, right?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review184751
-----------------------------------------------------------
On Sept. 7, 2017, 3:13 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 3:13 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
> src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
> src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
>
>
> Diff: https://reviews.apache.org/r/61183/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Sept. 7, 2017, 1:32 a.m., Jie Yu wrote:
> > src/slave/slave.hpp
> > Lines 658 (patched)
> > <https://reviews.apache.org/r/61183/diff/5/?file=1816402#file1816402line658>
> >
> > I am not sure if keeping another field just for resoruce provider provided resources in the agent is a good idea or not.
> >
> > I'd much prefer we keep a single `totalResources` for regular resources (both resource provider provided or not), and `oversubscribedResources` only for oversubsribed resources.
The advantage of using a dedicated member is that with it it is possible to detect whether any resources came from resource providers. With that we can avoid sending `UpdateSlaveMessage`s e.g., on reregistration when no resource providers registered (not sure how to do this without storing at least some data).
I'd suggest introducing this member as it captures everything we need.
> On Sept. 7, 2017, 1:32 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 1361 (patched)
> > <https://reviews.apache.org/r/61183/diff/5/?file=1816403#file1816403line1361>
> >
> > is it possible that agent is terminating?
This was a result of a wrong copy-paste.
I removed the `CHECK` here and below as we could just send the message in any possible state, like is already done for oversubscribed resources.
> On Sept. 7, 2017, 1:32 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 1370 (patched)
> > <https://reviews.apache.org/r/61183/diff/5/?file=1816403#file1816403line1370>
> >
> > what about checkpointed resources? I think we need to use `totalResources` (with checkpointed reosurces applied) here.
> >
> > Also, i checked the master handler for `UpdateSlaveMessage`. Should we rescind offer too (like the oversubscription case)? Add a TODO there?
Fixed the code to use the correct resources now.
re:master handling of `UpdateSlaveMessage` with `TOTAL`, it makes sense to rescind these offers as well. I pushed a minimal implementation, https://reviews.apache.org/r/62158/.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review184751
-----------------------------------------------------------
On Sept. 7, 2017, 5:13 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 5:13 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
> src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
> src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
>
>
> Diff: https://reviews.apache.org/r/61183/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review184751
-----------------------------------------------------------
src/slave/slave.hpp
Lines 658 (patched)
<https://reviews.apache.org/r/61183/#comment260963>
I am not sure if keeping another field just for resoruce provider provided resources in the agent is a good idea or not.
I'd much prefer we keep a single `totalResources` for regular resources (both resource provider provided or not), and `oversubscribedResources` only for oversubsribed resources.
src/slave/slave.cpp
Lines 1361 (patched)
<https://reviews.apache.org/r/61183/#comment260956>
is it possible that agent is terminating?
src/slave/slave.cpp
Lines 1370 (patched)
<https://reviews.apache.org/r/61183/#comment260955>
what about checkpointed resources? I think we need to use `totalResources` (with checkpointed reosurces applied) here.
Also, i checked the master handler for `UpdateSlaveMessage`. Should we rescind offer too (like the oversubscription case)? Add a TODO there?
- Jie Yu
On Sept. 6, 2017, 2:52 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 2:52 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04
> src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
> src/slave/slave.cpp df920ec07cd59c7ba6baccfc1c20ed3809f187d6
> src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
>
>
> Diff: https://reviews.apache.org/r/61183/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review185816
-----------------------------------------------------------
src/slave/slave.cpp
Lines 1272-1285 (patched)
<https://reviews.apache.org/r/61183/#comment262162>
This might be redundant depending on when we are able to instantiate the resource provider manager.
In another patch I make creating of the registrar dependent on the `SlaveID` which would force us to delay creating of the manager (which depends on the registrar) to somewhere here. We would in that case not yet have seen any updates.
- Benjamin Bannier
On Sept. 20, 2017, 4:24 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2017, 4:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
>
>
> Diff: https://reviews.apache.org/r/61183/diff/10/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Sept. 29, 2017, 5:33 a.m., Jie Yu wrote:
> > src/tests/oversubscription_tests.cpp
> > Lines 324-329 (original), 333-338 (patched)
> > <https://reviews.apache.org/r/61183/diff/14/?file=1838501#file1838501line333>
> >
> > Do you need to update those tests?
This particular test case checks that no update of oversubscribed agent resources is sent before the first `oversubscribed_resources_interval` has elapsed. The agent will however send its latest total on registration. We need check these independently to make sure they contain what we expect.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review186632
-----------------------------------------------------------
On Sept. 29, 2017, 11:14 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2017, 11:14 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
>
>
> Diff: https://reviews.apache.org/r/61183/diff/15/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review186632
-----------------------------------------------------------
src/slave/slave.cpp
Line 1278 (original), 1287 (patched)
<https://reviews.apache.org/r/61183/#comment263352>
Do you still need this?
src/slave/slave.cpp
Line 1358 (original), 1376 (patched)
<https://reviews.apache.org/r/61183/#comment263354>
Ditto.
src/tests/oversubscription_tests.cpp
Lines 324-329 (original), 333-338 (patched)
<https://reviews.apache.org/r/61183/#comment263355>
Do you need to update those tests?
- Jie Yu
On Sept. 28, 2017, 6:42 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2017, 6:42 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
>
>
> Diff: https://reviews.apache.org/r/61183/diff/14/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review190047
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/slave.cpp
Lines 1386 (patched)
<https://reviews.apache.org/r/61183/#comment267300>
Do you need to do the same (check agent capabilities)?
- Jie Yu
On Nov. 3, 2017, 3:21 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2017, 3:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
> src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
> src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
>
>
> Diff: https://reviews.apache.org/r/61183/diff/20/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Nov. 6, 2017, 6:51 p.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 1280-1287 (patched)
> > <https://reviews.apache.org/r/61183/diff/21/?file=1881892#file1881892line1280>
> >
> > It would be nice if we only did this when necessary - i.e., if there have, in fact, been additional RP registrations. The agent could keep an optional list of RPs which register/update during that period; I think it would be pretty easy to insert them in the handleResourceProviderMessage handler when the agent isn't registered. Here, and in reregistered().
> >
> > Do you think it's worth it?
Let's keep this as is for now. Since `UpdateSlaveMessage` only updates resource versions for added/changed RPs, the only effect it would have on the master side would be to invalidate outstanding offers for that agent. Let's optimize it when we need to, especially since, like you wrote, this might involve tracking additional state.
> On Nov. 6, 2017, 6:51 p.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 8529-8531 (patched)
> > <https://reviews.apache.org/r/61183/diff/21/?file=1881893#file1881893line8529>
> >
> > "of for the interaction with the usual oversubscription protocol" - I'm not sure what you're saying here, could you re-word?
`s/of/or/`
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review190177
-----------------------------------------------------------
On Nov. 7, 2017, 1:25 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2017, 1:25 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
> src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
> src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
>
>
> Diff: https://reviews.apache.org/r/61183/diff/22/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review190177
-----------------------------------------------------------
src/slave/slave.cpp
Lines 1280-1287 (patched)
<https://reviews.apache.org/r/61183/#comment267449>
It would be nice if we only did this when necessary - i.e., if there have, in fact, been additional RP registrations. The agent could keep an optional list of RPs which register/update during that period; I think it would be pretty easy to insert them in the handleResourceProviderMessage handler when the agent isn't registered. Here, and in reregistered().
Do you think it's worth it?
src/tests/slave_tests.cpp
Lines 8529-8531 (patched)
<https://reviews.apache.org/r/61183/#comment267450>
"of for the interaction with the usual oversubscription protocol" - I'm not sure what you're saying here, could you re-word?
src/tests/slave_tests.cpp
Lines 8536 (patched)
<https://reviews.apache.org/r/61183/#comment267451>
s/an a/and a/
src/tests/slave_tests.cpp
Lines 8543-8546 (patched)
<https://reviews.apache.org/r/61183/#comment267452>
Could you comment on the reason for setting `authenticate_http_readwrite = false` as well?
- Greg Mann
On Nov. 6, 2017, 12:38 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2017, 12:38 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
> src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
> src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
>
>
> Diff: https://reviews.apache.org/r/61183/diff/21/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review190335
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Nov. 7, 2017, 12:25 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2017, 12:25 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
> src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
> src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
>
>
> Diff: https://reviews.apache.org/r/61183/diff/22/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Nov. 7, 2017, 11 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed comment by Jie.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
src/slave/slave.cpp c10823985154bac19f8952b94311a03b2b9b4ea1
src/tests/slave_tests.cpp f9c2e6b41bdbc54ee0d8d06a2a41c92b7a1156cc
Diff: https://reviews.apache.org/r/61183/diff/23/
Changes: https://reviews.apache.org/r/61183/diff/22-23/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Nov. 7, 2017, 1:25 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed comments by Greg.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
Diff: https://reviews.apache.org/r/61183/diff/22/
Changes: https://reviews.apache.org/r/61183/diff/21-22/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Nov. 6, 2017, 1:38 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed comment from Jie; rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
Diff: https://reviews.apache.org/r/61183/diff/21/
Changes: https://reviews.apache.org/r/61183/diff/20-21/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Nov. 3, 2017, 4:21 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
src/slave/slave.cpp 79ee163b41ade93cae1054985379d61faf6a081a
src/tests/slave_tests.cpp 9c2db7adf45af4209ecc475d938ce4b77c5a3ab4
Diff: https://reviews.apache.org/r/61183/diff/20/
Changes: https://reviews.apache.org/r/61183/diff/19-20/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Nov. 2, 2017, 6:47 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Only send updates when agent is marked resource provider capable.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
src/slave/slave.cpp 337083dbe60bba2d3773b785bdceeaf0b8fcd070
src/tests/slave_tests.cpp aae5e6021b46793fa2b871aaa738f61c8cfb88ce
Diff: https://reviews.apache.org/r/61183/diff/19/
Changes: https://reviews.apache.org/r/61183/diff/18-19/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Nov. 2, 2017, 2:57 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Reopened and rebased as patch was reverted in `a3faf6cf854cd64271bff7246ace13acfa37e731`.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7
src/slave/slave.cpp 337083dbe60bba2d3773b785bdceeaf0b8fcd070
src/tests/oversubscription_tests.cpp d262bbe06787f70427f4f7820e64891c7d1b5552
src/tests/slave_tests.cpp aae5e6021b46793fa2b871aaa738f61c8cfb88ce
Diff: https://reviews.apache.org/r/61183/diff/18/
Changes: https://reviews.apache.org/r/61183/diff/17-18/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review186984
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Sept. 29, 2017, 9:14 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2017, 9:14 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp bf85baf8b7f7ebf52afa25214e71278f18dc9b30
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp 2ff6daba236972a732618257b25dc3e7fa4dec60
>
>
> Diff: https://reviews.apache.org/r/61183/diff/16/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 29, 2017, 11:14 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Removed redundant updates.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/15/
Changes: https://reviews.apache.org/r/61183/diff/14-15/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 28, 2017, 8:42 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/14/
Changes: https://reviews.apache.org/r/61183/diff/13-14/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 25, 2017, 9:42 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed comments from Jan, improved comments.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/13/
Changes: https://reviews.apache.org/r/61183/diff/12-13/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review186112
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/slave.cpp
Lines 6619 (patched)
<https://reviews.apache.org/r/61183/#comment262543>
This is missing a `return;`.
src/slave/slave.cpp
Lines 6620 (patched)
<https://reviews.apache.org/r/61183/#comment262544>
Remove this line.
- Jan Schlicht
On Sept. 25, 2017, 12:28 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2017, 12:28 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
>
>
> Diff: https://reviews.apache.org/r/61183/diff/12/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 25, 2017, 12:28 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 75e2e25c438637e2b1731f9fd01b6fe297687c50
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/12/
Changes: https://reviews.apache.org/r/61183/diff/11-12/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review185932
-----------------------------------------------------------
See comments below. I presume you won't need to update the oversubscription tests if you take my suggestion below? Thoughts?
src/slave/slave.cpp
Lines 1272-1284 (patched)
<https://reviews.apache.org/r/61183/#comment262262>
Can you add a comment here saying that we need to send the message here because some local resource provider might register with the resource provider manager while agent is registering with the master.
Also, I realize another issue: what if the new agent send this message to an old master? will the old master remove all the oversubscribed resources (thus unnecssarily rescind offers? Maybe we do need to solve the `TODO` here? That means probably we need to make `UpdateSlaveMessage::Type` a mask, rather than an enum so that we can update both total and oversubscribed resources in one message?
```
message UpdateSlaveMessage {
message ResourceCategories {
optional boolean total = 1;
optional boolean oversubscribed = 2;
}
// NOTE: If not set, means update oversubscribed resources.
optional ResourceCategories resource_categories = 3;
repeated Resource oversubscribed_resources = 2;
repeated Resource total_resources = 4;
}
```
src/slave/slave.cpp
Lines 1366 (patched)
<https://reviews.apache.org/r/61183/#comment262263>
Ditto on adding some comments here.
- Jie Yu
On Sept. 21, 2017, 7:24 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2017, 7:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
>
>
> Diff: https://reviews.apache.org/r/61183/diff/11/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 21, 2017, 9:24 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/11/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 21, 2017, 2:59 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/11/
Changes: https://reviews.apache.org/r/61183/diff/10-11/
Testing
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 20, 2017, 4:24 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed Jie's comments, mainly
* Always send out updated total. This simplifies the agent code a lot and has minimal effects as with the created https://reviews.apache.org/r/62438/ the master would ignore redundant agent updates. This change requires adjusting a few oversubscription tests that were only expecting a single update.
* Improved logging in a few places.
Repository: mesos
Description (updated)
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82
src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
Diff: https://reviews.apache.org/r/61183/diff/10/
Changes: https://reviews.apache.org/r/61183/diff/9-10/
Testing (updated)
-------
Tested on a number of platforms on internal CI.
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 19, 2017, 6:53 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
src/slave/slave.cpp 391a0d6db29660d5eb38eab367a2ccf9d453e69d
src/tests/slave_tests.cpp 0a578ff0cf264999c95db8ccd16e6e52807fa070
Diff: https://reviews.apache.org/r/61183/diff/9/
Changes: https://reviews.apache.org/r/61183/diff/8-9/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Sept. 19, 2017, 12:03 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 6669 (patched)
> > <https://reviews.apache.org/r/61183/diff/8/?file=1827893#file1827893line6669>
> >
> > add a `break` for the first level switch? and a default?
I added a `break` here.
I did not add a `default` branch as we want to explicitly trigger a compiler warning about uncovered cases here should another message type be added in the future.
> On Sept. 19, 2017, 12:03 a.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp
> > Lines 8357 (patched)
> > <https://reviews.apache.org/r/61183/diff/8/?file=1827894#file1827894line8357>
> >
> > can we get rid of the process:: prefix?
All types from `process::http` in this file are fully qualified (e.g., also `process::http::Request`). I'll submit an additional RR to tidy this up (https://reviews.apache.org/r/62439/).
> On Sept. 19, 2017, 12:03 a.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp
> > Lines 8372 (patched)
> > <https://reviews.apache.org/r/61183/diff/8/?file=1827894#file1827894line8372>
> >
> > Do we need the `mesos::` prefix?
I believe sadly we do. We for sure what to distinguish unversioned from e.g., `v1` types. On the other hand in tests we have a namespace `mesos::internal::v1` and just e.g., `v1::resource_provider::Event::Subscribed` is looked up in that one instead of `mesos::v1`.
I guess an alternative would be to define aliases for `v1` types and subnamespaces, but I don't see that being done currently (and it IMO seems to make things even more convoluted).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review185583
-----------------------------------------------------------
On Sept. 20, 2017, 4:24 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2017, 4:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
> src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82
> src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca
> src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723
>
>
> Diff: https://reviews.apache.org/r/61183/diff/10/
>
>
> Testing
> -------
>
> Tested on a number of platforms on internal CI.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/#review185583
-----------------------------------------------------------
src/slave/slave.cpp
Lines 1524 (patched)
<https://reviews.apache.org/r/61183/#comment261903>
Can you update for the re-registration case as well?
src/slave/slave.cpp
Lines 3505-3507 (patched)
<https://reviews.apache.org/r/61183/#comment261925>
I am trying very hard to think if this is problematic. It's very hard to reason about. My intuition told me that this might be problematic because `totalResources` might reflect some new update to the RP's resources that haven't synced with the master yet.
Given that `lastSyncedTotalResources` is just an optimization, I'll just don't do anything in this function.
src/slave/slave.cpp
Lines 6124-6126 (patched)
<https://reviews.apache.org/r/61183/#comment261902>
No need for this given that you'll update `lastSyncedTotalResources` before sending `RegisterSlaveMessage`?
src/slave/slave.cpp
Lines 6609 (patched)
<https://reviews.apache.org/r/61183/#comment261904>
Include the error message?
I would make this a LOG(ERROR) instead
src/slave/slave.cpp
Lines 6617 (patched)
<https://reviews.apache.org/r/61183/#comment261930>
I'd make this a LOG(INFO)
src/slave/slave.cpp
Lines 6620 (patched)
<https://reviews.apache.org/r/61183/#comment261927>
Let's add a parathesis to the `case` statement:
```
case ResourceProviderMessage::Type::UPDATE_TOTAL_RESOURCES: {
...
}
```
src/slave/slave.cpp
Lines 6630 (patched)
<https://reviews.apache.org/r/61183/#comment261926>
indentation?
src/slave/slave.cpp
Lines 6669 (patched)
<https://reviews.apache.org/r/61183/#comment261929>
add a `break` for the first level switch? and a default?
src/tests/slave_tests.cpp
Lines 8357 (patched)
<https://reviews.apache.org/r/61183/#comment261934>
can we get rid of the process:: prefix?
src/tests/slave_tests.cpp
Lines 8372 (patched)
<https://reviews.apache.org/r/61183/#comment261933>
Do we need the `mesos::` prefix?
- Jie Yu
On Sept. 15, 2017, 1:15 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2017, 1:15 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
>
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
>
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
>
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
>
>
> Diffs
> -----
>
> src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
> src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
> src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
> src/tests/slave_tests.cpp 0a578ff0cf264999c95db8ccd16e6e52807fa070
>
>
> Diff: https://reviews.apache.org/r/61183/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 15, 2017, 3:15 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
src/tests/slave_tests.cpp 0a578ff0cf264999c95db8ccd16e6e52807fa070
Diff: https://reviews.apache.org/r/61183/diff/8/
Changes: https://reviews.apache.org/r/61183/diff/7-8/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 8, 2017, 12:38 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Avoid explicitly tracking resource provider resources as suggested by Jie.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
Diff: https://reviews.apache.org/r/61183/diff/7/
Changes: https://reviews.apache.org/r/61183/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 5:13 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed Jie's comments.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521
src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd
src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
Diff: https://reviews.apache.org/r/61183/diff/6/
Changes: https://reviews.apache.org/r/61183/diff/5-6/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when
'ResourceProviderManager' updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61183/
-----------------------------------------------------------
(Updated Sept. 6, 2017, 4:52 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed Jan's offline comments.
Repository: mesos
Description
-------
The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.
In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).
Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.
To facilitate logging we add a stringification function for
'ResourceProviderMessage's.
Diffs (updated)
-----
src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04
src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094
src/slave/slave.cpp df920ec07cd59c7ba6baccfc1c20ed3809f187d6
src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd
Diff: https://reviews.apache.org/r/61183/diff/5/
Changes: https://reviews.apache.org/r/61183/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier