You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Thomas Marshall <tw...@gmail.com> on 2013/06/05 23:21:16 UTC
Re: Review Request: WDRF 2 - Add concept of adding/removing roles
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/
-----------------------------------------------------------
(Updated June 5, 2013, 9:21 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Updated from discussion with Ben H: added a weights command line flag to master so that masters can start with role weights already set.
Description
-------
Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
Diffs (updated)
-----
include/mesos/mesos.proto ece6559
src/master/allocator.hpp 78c75bb
src/master/flags.hpp f4ce8c1
src/master/hierarchical_allocator_process.hpp 1048a28
src/master/master.hpp 8e7b74c
src/master/master.cpp a2e4b90
src/tests/allocator_tests.cpp 32f0a90
src/tests/allocator_zookeeper_tests.cpp 1daaecd
src/tests/mesos.hpp fca41aa
src/tests/resource_offers_tests.cpp 3d5f02d
Diff: https://reviews.apache.org/r/11206/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/
-----------------------------------------------------------
(Updated June 18, 2013, 9:12 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Vinod's review.
Description
-------
Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
This addresses bug MESOS-504.
https://issues.apache.org/jira/browse/MESOS-504
Diffs (updated)
-----
src/master/allocator.hpp 78c75bb
src/master/flags.hpp f4ce8c1
src/master/hierarchical_allocator_process.hpp 1048a28
src/master/master.hpp 86c5232
src/master/master.cpp 60c6d4f
src/messages/messages.proto 2c196ee
src/tests/allocator_tests.cpp 32f0a90
src/tests/allocator_zookeeper_tests.cpp 1daaecd
src/tests/mesos.hpp fca41aa
src/tests/resource_offers_tests.cpp 3d5f02d
Diff: https://reviews.apache.org/r/11206/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review22070
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On June 13, 2013, 7:24 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 13, 2013, 7:24 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/messages/messages.proto 2c196ee
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Thomas Marshall <tw...@gmail.com>.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 248-251
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line248>
> >
> > Why do we always want to allow a default role?
>
> Thomas Marshall wrote:
> We did this to minimize the impact of these changes on people who don't care about roles or hierarchical allocation, but I agree that we don't always want a default role. Maybe the solution is to add the default role if no --role flag is present, and then allow people who use roles to turn on the default role by including a '*' in their --roles flag.
Ben and I talked, and we're going to leave the default role turned on all of the time. If people don't want it, they can set all of the resources on all of the slaves to be reserved for specific roles (which they would have to do anyways without a default role) and get the same effect, but we figure most people will want the default role most of the time precisely because it doesn't require you to mark all of the resources in the entire cluster with a specific role.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 259
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line259>
> >
> > why don't we call the member 'role' instead of 'name'?
> >
> > I think it is a bit confusing that we call the flag tokens 'role's, but we have a class called 'Role' and an attribute of RoleInfo called 'name' which is actually a 'role'.
> >
>
> Thomas Marshall wrote:
> I think that 'name' is cleaner, since it's in line with the way that Resource, Attribute, FrameworkInfo, ExecutorInfo, and TaskInfo all have 'name' fields.
Ben agrees with me.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 598-602
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line598>
> >
> > Don't we have to check this in re-register too?
>
> Thomas Marshall wrote:
> Good point. This also brings up the question of what we should do if a framework tries to reregister with a different role. We could check this by sending FrameworkInfos from the slaves to the master when they reregister, but that gets complicated if a framework reregisters before any slaves that it has tasks running on reregister, or if a framework that doesn't have any currently running tasks, and therefore none of the slaves have a copy of its FrameworkInfo, reregisters.
We're going to leave the problem of frameworks reregistering with different roles for when we do security.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21872
-----------------------------------------------------------
On June 13, 2013, 7:24 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 13, 2013, 7:24 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/messages/messages.proto 2c196ee
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Vinod Kone <vi...@gmail.com>.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 248-251
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line248>
> >
> > Why do we always want to allow a default role?
>
> Thomas Marshall wrote:
> We did this to minimize the impact of these changes on people who don't care about roles or hierarchical allocation, but I agree that we don't always want a default role. Maybe the solution is to add the default role if no --role flag is present, and then allow people who use roles to turn on the default role by including a '*' in their --roles flag.
>
> Thomas Marshall wrote:
> Ben and I talked, and we're going to leave the default role turned on all of the time. If people don't want it, they can set all of the resources on all of the slaves to be reserved for specific roles (which they would have to do anyways without a default role) and get the same effect, but we figure most people will want the default role most of the time precisely because it doesn't require you to mark all of the resources in the entire cluster with a specific role.
I am mainly concerned about the possibility of disallowing a default role. I though the roles were ever gonna be specified on the master's cmd flags. But sounds like there is a complementary role flag for slaves that allows us to disable the default role? I guess thats in a different review?
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 598-602
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line598>
> >
> > Don't we have to check this in re-register too?
>
> Thomas Marshall wrote:
> Good point. This also brings up the question of what we should do if a framework tries to reregister with a different role. We could check this by sending FrameworkInfos from the slaves to the master when they reregister, but that gets complicated if a framework reregisters before any slaves that it has tasks running on reregister, or if a framework that doesn't have any currently running tasks, and therefore none of the slaves have a copy of its FrameworkInfo, reregisters.
>
> Thomas Marshall wrote:
> We're going to leave the problem of frameworks reregistering with different roles for when we do security.
I was mostly thinking this is something that could go into the Master's registry. But I'm fine with punting on it for now.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21872
-----------------------------------------------------------
On June 18, 2013, 9:12 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 18, 2013, 9:12 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/messages/messages.proto 2c196ee
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 248-251
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line248>
> >
> > Why do we always want to allow a default role?
>
> Thomas Marshall wrote:
> We did this to minimize the impact of these changes on people who don't care about roles or hierarchical allocation, but I agree that we don't always want a default role. Maybe the solution is to add the default role if no --role flag is present, and then allow people who use roles to turn on the default role by including a '*' in their --roles flag.
>
> Thomas Marshall wrote:
> Ben and I talked, and we're going to leave the default role turned on all of the time. If people don't want it, they can set all of the resources on all of the slaves to be reserved for specific roles (which they would have to do anyways without a default role) and get the same effect, but we figure most people will want the default role most of the time precisely because it doesn't require you to mark all of the resources in the entire cluster with a specific role.
>
> Vinod Kone wrote:
> I am mainly concerned about the possibility of disallowing a default role. I though the roles were ever gonna be specified on the master's cmd flags. But sounds like there is a complementary role flag for slaves that allows us to disable the default role? I guess thats in a different review?
TL;DR; I don't think we'd ever want to disallow a default role.
Having a _special_ "default" role (i.e., '*') enables launching frameworks without requiring reconfiguring the master. I don't think we want to configure reservations for every possible authenticated user. Rather, if a user runs in the "default" role they can be allocated whatever resources are available to the default role (which may be none!) without requiring some human intervention first.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21872
-----------------------------------------------------------
On June 18, 2013, 9:12 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 18, 2013, 9:12 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/messages/messages.proto 2c196ee
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Thomas Marshall <tw...@gmail.com>.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 248-251
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line248>
> >
> > Why do we always want to allow a default role?
We did this to minimize the impact of these changes on people who don't care about roles or hierarchical allocation, but I agree that we don't always want a default role. Maybe the solution is to add the default role if no --role flag is present, and then allow people who use roles to turn on the default role by including a '*' in their --roles flag.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 598-602
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line598>
> >
> > Don't we have to check this in re-register too?
Good point. This also brings up the question of what we should do if a framework tries to reregister with a different role. We could check this by sending FrameworkInfos from the slaves to the master when they reregister, but that gets complicated if a framework reregisters before any slaves that it has tasks running on reregister, or if a framework that doesn't have any currently running tasks, and therefore none of the slaves have a copy of its FrameworkInfo, reregisters.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/tests/allocator_tests.cpp, lines 79-81
> > <https://reviews.apache.org/r/11206/diff/6/?file=304919#file304919line79>
> >
> > Can we add tests for when frameworks connect with a role that is known/unknown to the master?
That's included in the 'tests' review later in this review chain.
> On June 13, 2013, 11:44 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 259
> > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line259>
> >
> > why don't we call the member 'role' instead of 'name'?
> >
> > I think it is a bit confusing that we call the flag tokens 'role's, but we have a class called 'Role' and an attribute of RoleInfo called 'name' which is actually a 'role'.
> >
I think that 'name' is cleaner, since it's in line with the way that Resource, Attribute, FrameworkInfo, ExecutorInfo, and TaskInfo all have 'name' fields.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21872
-----------------------------------------------------------
On June 13, 2013, 7:24 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 13, 2013, 7:24 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/messages/messages.proto 2c196ee
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21872
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/11206/#comment45171>
const?
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment45174>
Why do we always want to allow a default role?
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment45175>
why don't we call the member 'role' instead of 'name'?
I think it is a bit confusing that we call the flag tokens 'role's, but we have a class called 'Role' and an attribute of RoleInfo called 'name' which is actually a 'role'.
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment45176>
Don't we have to check this in re-register too?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/11206/#comment45177>
Can we add tests for when frameworks connect with a role that is known/unknown to the master?
- Vinod Kone
On June 13, 2013, 7:24 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 13, 2013, 7:24 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/messages/messages.proto 2c196ee
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of valid roles
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/
-----------------------------------------------------------
(Updated June 13, 2013, 7:24 p.m.)
Review request for mesos and Benjamin Hindman.
Summary (updated)
-----------------
WDRF 2 - Add concept of valid roles
Description
-------
Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
This addresses bug MESOS-504.
https://issues.apache.org/jira/browse/MESOS-504
Diffs
-----
src/master/allocator.hpp 78c75bb
src/master/flags.hpp f4ce8c1
src/master/hierarchical_allocator_process.hpp 1048a28
src/master/master.hpp 86c5232
src/master/master.cpp 60c6d4f
src/messages/messages.proto 2c196ee
src/tests/allocator_tests.cpp 32f0a90
src/tests/allocator_zookeeper_tests.cpp 1daaecd
src/tests/mesos.hpp fca41aa
src/tests/resource_offers_tests.cpp 3d5f02d
Diff: https://reviews.apache.org/r/11206/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request: WDRF 2 - Add concept of adding/removing roles
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/
-----------------------------------------------------------
(Updated June 13, 2013, 7:22 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Ben's review.
Description
-------
Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
This addresses bug MESOS-504.
https://issues.apache.org/jira/browse/MESOS-504
Diffs (updated)
-----
src/master/allocator.hpp 78c75bb
src/master/flags.hpp f4ce8c1
src/master/hierarchical_allocator_process.hpp 1048a28
src/master/master.hpp 86c5232
src/master/master.cpp 60c6d4f
src/messages/messages.proto 2c196ee
src/tests/allocator_tests.cpp 32f0a90
src/tests/allocator_zookeeper_tests.cpp 1daaecd
src/tests/mesos.hpp fca41aa
src/tests/resource_offers_tests.cpp 3d5f02d
Diff: https://reviews.apache.org/r/11206/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request: WDRF 2 - Add concept of adding/removing roles
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/
-----------------------------------------------------------
(Updated June 12, 2013, 11:40 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Added bug.
Description
-------
Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
This addresses bug MESOS-504.
https://issues.apache.org/jira/browse/MESOS-504
Diffs
-----
include/mesos/mesos.proto 8cbcd9a
src/master/allocator.hpp 78c75bb
src/master/flags.hpp f4ce8c1
src/master/hierarchical_allocator_process.hpp 1048a28
src/master/master.hpp 86c5232
src/master/master.cpp 60c6d4f
src/tests/allocator_tests.cpp 32f0a90
src/tests/allocator_zookeeper_tests.cpp 1daaecd
src/tests/mesos.hpp fca41aa
src/tests/resource_offers_tests.cpp 3d5f02d
Diff: https://reviews.apache.org/r/11206/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request: WDRF 2 - Add concept of adding/removing roles
Posted by Thomas Marshall <tw...@gmail.com>.
> On June 12, 2013, 11:13 p.m., Benjamin Hindman wrote:
> > src/master/master.hpp, lines 181-184
> > <https://reviews.apache.org/r/11206/diff/4/?file=301197#file301197line181>
> >
> > Some thoughts on the 'updateRole' and 'removeRole' "family" of functions. First, 'removeRole' is never called, so why add it? Second, the semantics around updating a role are unclear to me. If we update a role's weight, does hat mean that the allocator needs to take appropriate action immediately and re-allocate as necessary? I think there are a lot of questions around these semantics that I don't think we need to tackle right now. Why not just pass the list of roles to the allocator when we initialize it for now? That is, in initialize, construct a map of RoleInfo's, update those as appropriate based on --weights, save those to the master and pass them to the allocator? This seems simple and straightforward and doesn't introduce complexities that I think we should punt for now.
This is left over from before when there was an endpoint for updating roles. Removed.
> On June 12, 2013, 11:13 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, lines 399-402
> > <https://reviews.apache.org/r/11206/diff/4/?file=301198#file301198line399>
> >
> > You could clean this up using 'strings::pairs'. The caveat here is that it ignores malformed input. I like validating the input so I suggest:
> >
> > (1) Updating strings::pairs to return a Try<std::map<...>> and use it, outputting the error as necessary.
> >
> > (2) Continue to do it the way you are doing (but creating a Github issue at 3rdparty/stout would be nice).
The problem with having strings::pairs return a Try is that the one place we are actually using it right now, process::http::Response::accepts, relies on it simply ignoring invalid pairs in order to work. I don't know what the right work around for this is, so I'll just leave it all as is for now.
> On June 12, 2013, 11:13 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 1700
> > <https://reviews.apache.org/r/11206/diff/4/?file=301198#file301198line1700>
> >
> > Yes, you've made sure the role is valid in Master::registerFramework, but no reason not to put a CHECK here for your fellow developers. In fact, you can even add a comment above the CHECK pointing to the validation in Master::registerFramework. Another question, what happens if a slave connects with a role that the master doesn't know about?
I'm not sure what to do about the slave registering with an invalid role situation. Obviously we don't want a situation where the resources dedicated to the invalid role just silently sit there idle, so we could either have the master log an error and put the resources into the default pool, or we could create a slave registration error message to send back so that the slave can exit (this second option seems more in line with what you said about malformed --weights flags).
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21726
-----------------------------------------------------------
On June 12, 2013, 11:40 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 12, 2013, 11:40 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> This addresses bug MESOS-504.
> https://issues.apache.org/jira/browse/MESOS-504
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8cbcd9a
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of adding/removing roles
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/#review21726
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/11206/#comment44908>
After reviewing this code I'm starting to like the general purposeness of 'weight' and I think our comments should emphasize this. In particular, we should basically explain 'weight' as a way to "prioritize" roles, and that certain allocation algorithms, such as DRF, can use the weight information to allocate appropriately. Doing so means that future algorithms that might also want to "prioritize" roles can use weights as well. How does that sound?
include/mesos/mesos.proto
<https://reviews.apache.org/r/11206/#comment44906>
Do we need to expose this to framework developers? This seems like an internal structure for implementation purposes.
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/11206/#comment44899>
s/framewworks/frameworks/
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/11206/#comment44900>
s/CHECK (/CHECK(/
src/master/master.hpp
<https://reviews.apache.org/r/11206/#comment45021>
Some thoughts on the 'updateRole' and 'removeRole' "family" of functions. First, 'removeRole' is never called, so why add it? Second, the semantics around updating a role are unclear to me. If we update a role's weight, does hat mean that the allocator needs to take appropriate action immediately and re-allocate as necessary? I think there are a lot of questions around these semantics that I don't think we need to tackle right now. Why not just pass the list of roles to the allocator when we initialize it for now? That is, in initialize, construct a map of RoleInfo's, update those as appropriate based on --weights, save those to the master and pass them to the allocator? This seems simple and straightforward and doesn't introduce complexities that I think we should punt for now.
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment44895>
You could clean this up using 'strings::pairs'. The caveat here is that it ignores malformed input. I like validating the input so I suggest:
(1) Updating strings::pairs to return a Try<std::map<...>> and use it, outputting the error as necessary.
(2) Continue to do it the way you are doing (but creating a Github issue at 3rdparty/stout would be nice).
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment44896>
Two things:
(1) Let's be nice to our users and fail early here via EXIT(1) rather than forcing them to read through the log file to find out that they had malformed flag input. We should probably do this in more places in the codebase, but no reason not to do this for new places.
(2) Let's be a bit more explicit for our users and explain that the either (a) the input was not of the form 'foo=1,bar=2' or (b) a role was not included in the 'roles' flag.
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment44897>
Yes, you've made sure the role is valid in Master::registerFramework, but no reason not to put a CHECK here for your fellow developers. In fact, you can even add a comment above the CHECK pointing to the validation in Master::registerFramework. Another question, what happens if a slave connects with a role that the master doesn't know about?
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment44901>
roles[role] = new Role(roleInfo);
src/master/master.cpp
<https://reviews.apache.org/r/11206/#comment44902>
Why the bool return code?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/11206/#comment44898>
Why the extra newline here (and in the rest of the tests in this diff)? This is not the convention in the rest of the tests.
- Benjamin Hindman
On June 12, 2013, 5:49 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11206/
> -----------------------------------------------------------
>
> (Updated June 12, 2013, 5:49 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
>
> So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
>
> This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8cbcd9a
> src/master/allocator.hpp 78c75bb
> src/master/flags.hpp f4ce8c1
> src/master/hierarchical_allocator_process.hpp 1048a28
> src/master/master.hpp 86c5232
> src/master/master.cpp 60c6d4f
> src/tests/allocator_tests.cpp 32f0a90
> src/tests/allocator_zookeeper_tests.cpp 1daaecd
> src/tests/mesos.hpp fca41aa
> src/tests/resource_offers_tests.cpp 3d5f02d
>
> Diff: https://reviews.apache.org/r/11206/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: WDRF 2 - Add concept of adding/removing roles
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11206/
-----------------------------------------------------------
(Updated June 12, 2013, 5:49 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Added a provision to ignore zero or negative weights.
Description
-------
Previously when we were doing hierarchical allocation by user, we created and removed user pools for allocation based on what users had frameworks currently running in the cluster. However, with the role abstraction it makes sense to have more persistence than that, especially once we add weights - if you set the weight for a role, you want the allocator to remember it even if there aren't any frameworks for that role currently running.
So, I decided that it made sense to create a concept of specific roles that are allowable in the cluster. With this patch, its only possible to pass roles in to the master as a command line flag (to ease what I assume will be the common case - clusters with relatively static sets of roles), but a future wdrf patch will add http endpoints to add, remove, and update roles.
This patch also enforces that frameworks register with valid role (this won't affect people who don't care about roles, since there's always the "*" role, which is the default for frameworks that don't specify a role).
Diffs (updated)
-----
include/mesos/mesos.proto 8cbcd9a
src/master/allocator.hpp 78c75bb
src/master/flags.hpp f4ce8c1
src/master/hierarchical_allocator_process.hpp 1048a28
src/master/master.hpp 86c5232
src/master/master.cpp 60c6d4f
src/tests/allocator_tests.cpp 32f0a90
src/tests/allocator_zookeeper_tests.cpp 1daaecd
src/tests/mesos.hpp fca41aa
src/tests/resource_offers_tests.cpp 3d5f02d
Diff: https://reviews.apache.org/r/11206/diff/
Testing
-------
make check
Thanks,
Thomas Marshall