You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/06/02 20:03:39 UTC
Review Request 22151: Authorize launch tasks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Bugs: MESOS-1114
https://issues.apache.org/jira/browse/MESOS-1114
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
Diff: https://reviews.apache.org/r/22151/diff/
Testing
-------
make check.
Will write new tests and update this review or will create a new one.
Thanks,
Vinod Kone
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > First pass, didn't look at all of the code just yet. Higher level comments:
> >
> > (1) 'validateTask' is a bit confusing in terms of return value because it's actually doing two things (validation and authorization). If it doesn't pass validation, we return a failure. If it doesn't pass authorization, we return false. Can you add documentation related to this?
> >
> > (2) The task visitor ownership is really tricky now! Any plan there? At least, is it possible to use 'Shared' visitors to avoid needing to pass them in to _launchTasks?
> >
> > I will help with these two:
> >
> > (3) MESOS-1451: To clean things up a bit, we should remove the singular 'offer_id' field from LaunchTasksMessage and the corresponding logic inside launchTasks to deal with it, since the scheduler driver has been setting 'offer_ids' since 0.18.0 and we're now in 0.20.0.
> >
> > (4) MESOS-1452: I think we should reduce the room for mistakes around removeOffer (per previous conversations) so that it takes an enum that decides whether to USE, DISCARD, RESCIND.
(1) added a comment.
(2) let me think about this.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2115
> > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line2115>
> >
> > How about calling this 'futures' plural?
thats how it was originally named but changed it to singular because its a future of a list not a list of futures. reverted.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1992-2015
> > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line1992>
> >
> > Might be nice to remove the indentation:
> >
> > if (authorizer.isNone()) {
> > return true;
> > }
> >
> > ...
done.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 917
> > <https://reviews.apache.org/r/22151/diff/1/?file=601790#file601790line917>
> >
> > Why are they pending? Maybe add a bit to this comment explaining?
added comment.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1535
> > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line1535>
> >
> > Seems like we should avoid using the phrase "if we are here" as it seems redundant to me (there are two other cases in the diff).
done
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2275-2284
> > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line2275>
> >
> > Was protobuf::createStatusUpdate insufficient here?
yea. because createStatusUpdate() expects a slave id. added a todo for createStatusUpdate() to take an optional slave id.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/hierarchical_allocator_process.hpp, line 546
> > <https://reviews.apache.org/r/22151/diff/2/?file=602448#file602448line546>
> >
> > In the interest of minimizing the diff, do you need this and the other logging level changes below? Maybe a dependent review?
ok.
> On June 3, 2014, 5:52 p.m., Ben Mahler wrote:
> > src/master/hierarchical_allocator_process.hpp, lines 550-555
> > <https://reviews.apache.org/r/22151/diff/2/?file=602448#file602448line550>
> >
> > I don't understand the subtlety here, it looks like Master::_launchTasks doesn't call resourcesUnused if the framework was removed, something I'm missing that the comment should say?
you are right. i was just being overzealous here. reverted the check for framework.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review44574
-----------------------------------------------------------
On June 3, 2014, 5:25 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 5:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review44574
-----------------------------------------------------------
First pass, didn't look at all of the code just yet. Higher level comments:
(1) 'validateTask' is a bit confusing in terms of return value because it's actually doing two things (validation and authorization). If it doesn't pass validation, we return a failure. If it doesn't pass authorization, we return false. Can you add documentation related to this?
(2) The task visitor ownership is really tricky now! Any plan there? At least, is it possible to use 'Shared' visitors to avoid needing to pass them in to _launchTasks?
I will help with these two:
(3) MESOS-1451: To clean things up a bit, we should remove the singular 'offer_id' field from LaunchTasksMessage and the corresponding logic inside launchTasks to deal with it, since the scheduler driver has been setting 'offer_ids' since 0.18.0 and we're now in 0.20.0.
(4) MESOS-1452: I think we should reduce the room for mistakes around removeOffer (per previous conversations) so that it takes an enum that decides whether to USE, DISCARD, RESCIND.
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment78998>
Why are they pending? Maybe add a bit to this comment explaining?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79080>
Seems like we should avoid using the phrase "if we are here" as it seems redundant to me (there are two other cases in the diff).
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment78999>
Might be nice to remove the indentation:
if (authorizer.isNone()) {
return true;
}
...
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79075>
How about calling this 'futures' plural?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79078>
'await' will never fail the Future (we should just CHECK that), so the only case that's important here is discarded, but since we don't discard(), that's also not possible.
How about a CHECK_READY(future) rather than all the logic to deal with a case that cannot occur?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79073>
Was protobuf::createStatusUpdate insufficient here?
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/22151/#comment79061>
In the interest of minimizing the diff, do you need this and the other logging level changes below? Maybe a dependent review?
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/22151/#comment79063>
I don't understand the subtlety here, it looks like Master::_launchTasks doesn't call resourcesUnused if the framework was removed, something I'm missing that the comment should say?
- Ben Mahler
On June 3, 2014, 5:25 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 5:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > Can you file a bug for the (existing) executor exited race condition we discussed?
> >
> > I commented on another (regressive) bug that is different than the one we discussed.
filed https://issues.apache.org/jira/browse/MESOS-1466 for the existing bug.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 319
> > <https://reviews.apache.org/r/22151/diff/4/?file=605180#file605180line319>
> >
> > Seems odd to say "fails" in the non failure (invalid) case, what about clarifying:
> >
> > // Returns None if the task is valid.
> > // Returns a validation Error if the task is invalid.
> > // Returns a Failure if an authorization failure occurs.
done.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/master/hierarchical_allocator_process.hpp, lines 562-564
> > <https://reviews.apache.org/r/22151/diff/4/?file=605179#file605179line562>
> >
> > Per your comment earlier about library functions knowing about the callers, could this comment be changed to reflect the allocator semantics instead of the master's semantics?
n/a now.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.hpp, line 51
> > <https://reviews.apache.org/r/22151/diff/4/?file=605178#file605178line51>
> >
> > Oh earlier I meant just letting the reader of the TODO know why we would make the SlaveID optional, not have the library function know about the callers. For example, if Alice comes along and reads this TODO, there's no context to guide her.
SlaveID can be optional because StatusUpdate.SlaveID is optional. Updated the comment.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2017-2021
> > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line2017>
> >
> > Why not just pass 'user' and have the "Not authorized ..." string inside _authorize? Seems a bit strange to pass the error string?
> >
> > Indentation?
because it's used in role authorization too.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2061-2070
> > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line2061>
> >
> > There is a race here where we overcommit! This is different from the race I mentioned earlier:
> >
> > -> executor is running
> > -> validateTask, launchTask is now queued
> > -> executorExited!
> > -> launchTask unqueued, now we're adding the executor resources even though our validation did not!!
good catch. refactored the code to delay resource usage checking.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1984
> > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line1984>
> >
> > Do you want the const& here?
n/a now.
> On June 9, 2014, 11:26 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1515
> > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line1515>
> >
> > newline here?
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review45151
-----------------------------------------------------------
On June 10, 2014, 6:27 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 6:27 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
> src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review45151
-----------------------------------------------------------
Can you file a bug for the (existing) executor exited race condition we discussed?
I commented on another (regressive) bug that is different than the one we discussed.
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/22151/#comment79831>
Oh earlier I meant just letting the reader of the TODO know why we would make the SlaveID optional, not have the library function know about the callers. For example, if Alice comes along and reads this TODO, there's no context to guide her.
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/22151/#comment79834>
Per your comment earlier about library functions knowing about the callers, could this comment be changed to reflect the allocator semantics instead of the master's semantics?
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment79838>
Seems odd to say "fails" in the non failure (invalid) case, what about clarifying:
// Returns None if the task is valid.
// Returns a validation Error if the task is invalid.
// Returns a Failure if an authorization failure occurs.
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79850>
newline here?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79862>
Do you want the const& here?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79856>
Why not just pass 'user' and have the "Not authorized ..." string inside _authorize? Seems a bit strange to pass the error string?
Indentation?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79874>
There is a race here where we overcommit! This is different from the race I mentioned earlier:
-> executor is running
-> validateTask, launchTask is now queued
-> executorExited!
-> launchTask unqueued, now we're adding the executor resources even though our validation did not!!
- Ben Mahler
On June 7, 2014, 1:37 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 7, 2014, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
> src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.hpp, lines 51-52
> > <https://reviews.apache.org/r/22151/diff/5/?file=606524#file606524line51>
> >
> > Just curious, isn't this trivial to add? All callers would still work?
I wanted to make it optional and have a default of None() which means it needs to be at the end of non-optional arguments, which is a signature change.
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 326-327
> > <https://reviews.apache.org/r/22151/diff/5/?file=606526#file606526line326>
> >
> > Looks like this comment needs an update w.r.t. the return value.
done
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 337
> > <https://reviews.apache.org/r/22151/diff/5/?file=606526#file606526line337>
> >
> > how about calling this 'validationError'?
called it validationErrors in the .cpp. Kept "f" here to avoid wrapping.
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1563-1566
> > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1563>
> >
> > Maybe a little note here that that tasks are added to 'pendingTasks' after being validated and that's why this works? (Non-local reasoning is needed here to know why this 'pendingTasks' lookup works).
done
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1888-1889
> > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1888>
> >
> > because validators use 'pendingTasks' to determine that the executorInfos are all the same?
yes and also used by UniqueTaskID checker.
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1894-1895
> > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1894>
> >
> > There was a bit more reasoning to it than this, right? Using one continuation made it easier to prevent races with exited executors and it's less code too!
this was the necessary reason. what you mention is good to have reason :)
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2067
> > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line2067>
> >
> > 'validationErrors'?
done
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2142-2143
> > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line2142>
> >
> > Maybe a little note that launchTask below will add the executor? (non-local reasoning needed)
done
> On June 10, 2014, 10:10 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1929-1935
> > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1929>
> >
> > Maybe a TODO to:
> >
> > Not use the heap, make the visit operation const, use a vector, and use a fixed visitor list that gets created during initialization?
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review45184
-----------------------------------------------------------
On June 10, 2014, 6:27 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 6:27 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
> src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review45184
-----------------------------------------------------------
Ship it!
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22151/#comment79909>
Looks like a copy paste with a lot of unneeded stuff?
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/22151/#comment80088>
Just curious, isn't this trivial to add? All callers would still work?
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment80049>
Looks like this comment needs an update w.r.t. the return value.
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment80074>
how about calling this 'validationError'?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80054>
:)
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80057>
Maybe a little note here that that tasks are added to 'pendingTasks' after being validated and that's why this works? (Non-local reasoning is needed here to know why this 'pendingTasks' lookup works).
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80059>
because validators use 'pendingTasks' to determine that the executorInfos are all the same?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80076>
There was a bit more reasoning to it than this, right? Using one continuation made it easier to prevent races with exited executors and it's less code too!
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80060>
Maybe a TODO to:
Not use the heap, make the visit operation const, use a vector, and use a fixed visitor list that gets created during initialization?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80079>
'validationErrors'?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80080>
Should this message be prefixed differently for both cases?
"Authorization failed: ..."
and
"Invalid task: ..." (includes not authorized case)
This should help the framework developers understand the messages coming out.
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment80086>
Maybe a little note that launchTask below will add the executor? (non-local reasoning needed)
- Ben Mahler
On June 10, 2014, 6:27 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 6:27 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
> src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/
-----------------------------------------------------------
(Updated June 10, 2014, 11:31 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's. NNFR.
Bugs: MESOS-1114
https://issues.apache.org/jira/browse/MESOS-1114
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22151/diff/
Testing
-------
make check.
Will write new tests and update this review or will create a new one.
Thanks,
Vinod Kone
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/
-----------------------------------------------------------
(Updated June 10, 2014, 6:27 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments.
Bugs: MESOS-1114
https://issues.apache.org/jira/browse/MESOS-1114
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22151/diff/
Testing
-------
make check.
Will write new tests and update this review or will create a new one.
Thanks,
Vinod Kone
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/
-----------------------------------------------------------
(Updated June 7, 2014, 1:37 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments.
Bugs: MESOS-1114
https://issues.apache.org/jira/browse/MESOS-1114
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60
src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22151/diff/
Testing
-------
make check.
Will write new tests and update this review or will create a new one.
Thanks,
Vinod Kone
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 317-327
> > <https://reviews.apache.org/r/22151/diff/3/?file=603279#file603279line317>
> >
> > Here's a proposal to try to avoid mixing the meaning the failure conditions (invalid task information vs. authorization denied):
> >
> > // Returns the validation result for the task.
> > // Returns a failure if validation fails.
> > Future<Option<Invalid> > validateTask(...);
> >
> > This exposes the validation result as the type of the future. Since we probably don't want to bother creating an 'Invalid' type for now, we can re-use the 'Error' type as we did in a few other places:
> >
> > // Returns the validation result for the task.
> > // Returns a failure if validation fails.
> > Future<Option<Error> > validateTask(...);
> >
> > This looks familiar to the discussion in: https://reviews.apache.org/r/22162/
done
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 329-331
> > <https://reviews.apache.org/r/22151/diff/3/?file=603279#file603279line329>
> >
> > Should we document the failure case for launchTask?
done
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2145
> > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2145>
> >
> > Looks like we would want this check even if the framework is null?
pulled it up.
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2151
> > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2151>
> >
> > What about post-incrementing here?
> >
> > const TaskInfo& task = tasks[index++];
> >
> > Or:
> >
> > const TaskInfo& task = tasks[index];
> > index++;
can't do because we need to increment index even if the 'future' failed.
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2153-2154
> > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2153>
> >
> > Could we add a TODO related to how we can achieve a synchronous launchTask->_launchTask instead of an asynchronous launchTask... ->_launchTasks?
> >
> > Might simplify things a bit and we wouldn't need to worry about this check here.
done
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2146
> > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2146>
> >
> > size_t?
done
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1476
> > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line1476>
> >
> > The fact that this code has non-local knowledge (that the task is placed inside pendingTasks before any further validation takes place) is really tricky and seems error prone, it seems safer to just keep the 'ids' here for now?
> >
> > FWIW it seems the executor ids were left in the ResourceUsageChecker as well.
Unfortunately we cannot get away from not depending on non-local knowledge. Example: If launchTasks1(), launchTasks2(), _launchTasks1() happen in this order, local 'ids' is of no use to detect a duplicate id. I left executor ids asis in ResourceUsageChecker as an optimization (to use a hashmap instead of having to loop through pendingTasks to find the executor id). I will remove the optmization. I want to get to a place where the validators don't hold local state (the last remnant is 'usedResources' in ResourceUsageChecker).
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 341
> > <https://reviews.apache.org/r/22151/diff/3/?file=603279#file603279line341>
> >
> > Thoughts on using shared_ptr for the TaskInfoVisitors? Might help avoid a memory leak and is a quick way to simplify things a bit.
done
> On June 5, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.hpp, line 51
> > <https://reviews.apache.org/r/22151/diff/3/?file=603277#file603277line51>
> >
> > // ... for updates created by the master?
don't want a util function to know about its callers.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review44833
-----------------------------------------------------------
On June 3, 2014, 11 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96
> src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review44833
-----------------------------------------------------------
Now I just need to look at the tests next! :)
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/22151/#comment79391>
// ... for updates created by the master?
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment79396>
Here's a proposal to try to avoid mixing the meaning the failure conditions (invalid task information vs. authorization denied):
// Returns the validation result for the task.
// Returns a failure if validation fails.
Future<Option<Invalid> > validateTask(...);
This exposes the validation result as the type of the future. Since we probably don't want to bother creating an 'Invalid' type for now, we can re-use the 'Error' type as we did in a few other places:
// Returns the validation result for the task.
// Returns a failure if validation fails.
Future<Option<Error> > validateTask(...);
This looks familiar to the discussion in: https://reviews.apache.org/r/22162/
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment79446>
Should we document the failure case for launchTask?
src/master/master.hpp
<https://reviews.apache.org/r/22151/#comment79447>
Thoughts on using shared_ptr for the TaskInfoVisitors? Might help avoid a memory leak and is a quick way to simplify things a bit.
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79449>
The fact that this code has non-local knowledge (that the task is placed inside pendingTasks before any further validation takes place) is really tricky and seems error prone, it seems safer to just keep the 'ids' here for now?
FWIW it seems the executor ids were left in the ResourceUsageChecker as well.
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79453>
If we don't end up using shared_ptr, can't this cleanup just be done once at the start of _launchTasks?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79456>
Looks like we would want this check even if the framework is null?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79457>
size_t?
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79459>
What about post-incrementing here?
const TaskInfo& task = tasks[index++];
Or:
const TaskInfo& task = tasks[index];
index++;
src/master/master.cpp
<https://reviews.apache.org/r/22151/#comment79458>
Could we add a TODO related to how we can achieve a synchronous launchTask->_launchTask instead of an asynchronous launchTask... ->_launchTasks?
Might simplify things a bit and we wouldn't need to worry about this check here.
- Ben Mahler
On June 3, 2014, 11 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96
> src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/
-----------------------------------------------------------
(Updated June 3, 2014, 11 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased.
Bugs: MESOS-1114
https://issues.apache.org/jira/browse/MESOS-1114
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96
src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1
src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22151/diff/
Testing
-------
make check.
Will write new tests and update this review or will create a new one.
Thanks,
Vinod Kone
Re: Review Request 22151: Authorize launch tasks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/
-----------------------------------------------------------
(Updated June 3, 2014, 5:25 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
some bug fixes that showed up as i wrote more tests.
i added 2 tests in this review. more involved tests are in the next review.
Bugs: MESOS-1114
https://issues.apache.org/jira/browse/MESOS-1114
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96
src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22151/diff/
Testing
-------
make check.
Will write new tests and update this review or will create a new one.
Thanks,
Vinod Kone
Re: Review Request 22151: Authorize launch tasks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22151/#review44539
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22148, 22149, 22150, 22151]
All tests passed.
- Mesos ReviewBot
On June 2, 2014, 6:03 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22151/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 6:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1114
> https://issues.apache.org/jira/browse/MESOS-1114
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/hierarchical_allocator_process.hpp 0c5e2e050cad96fafaf136232bd255b0ae3038cd
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22151/diff/
>
>
> Testing
> -------
>
> make check.
>
> Will write new tests and update this review or will create a new one.
>
>
> Thanks,
>
> Vinod Kone
>
>