You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/11/10 00:42:12 UTC
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 11:42 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Updated Summary & Description.
Summary (updated)
-----------------
[3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description (updated)
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs
-----
src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 13, 2015, 11:11 p.m., Jie Yu wrote:
> > src/master/master.cpp, lines 2737-2738
> > <https://reviews.apache.org/r/39987/diff/5/?file=1124800#file1124800line2737>
> >
> > This temp variable does not seem to be necessary. Can you just do:
> >
> > ```
> > LOG(INFO) << "Authorizing principal '"
> > << (principal.isSome() ? principal.get() : "ANY")
> > << "' to reserve resources";
> > ```
The temp variable isn't strictly necessary, but it does save us from extra checks of `principal.isSome()`. We have to do a little extra work either way: either we allocate a string to save what we find in `principal` the first time, or we have to check `principal.isSome()` multiple times. I don't know for sure, but my intuition is that checking `principal.isSome()` an extra time is more efficient, so I've switched to your suggestion. Thanks!
> On Nov. 13, 2015, 11:11 p.m., Jie Yu wrote:
> > src/master/master.cpp, line 2754
> > <https://reviews.apache.org/r/39987/diff/5/?file=1124800#file1124800line2754>
> >
> > We don't use tailing period for log messages. Please do a sweep to fix all of them.
I changed this one; I'll make a sweep through the rest of the reviews to check for others, and mark this issue as Fixed afterward.
> On Nov. 13, 2015, 11:11 p.m., Jie Yu wrote:
> > src/master/master.cpp, lines 2794-2799
> > <https://reviews.apache.org/r/39987/diff/5/?file=1124800#file1124800line2794>
> >
> > Can you just stringify(unreserve.resources()) below when calling LOG(INFO)?
Yep, great call! Done.
> On Nov. 13, 2015, 11:11 p.m., Jie Yu wrote:
> > src/master/master.cpp, lines 2802-2804
> > <https://reviews.apache.org/r/39987/diff/5/?file=1124800#file1124800line2802>
> >
> > Ditto above. No need for the 'principalString' temp variable.
Ditto above.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review106518
-----------------------------------------------------------
On Nov. 13, 2015, 11:32 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2015, 11:32 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
> src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review106518
-----------------------------------------------------------
src/master/master.cpp (lines 2737 - 2738)
<https://reviews.apache.org/r/39987/#comment165223>
This temp variable does not seem to be necessary. Can you just do:
```
LOG(INFO) << "Authorizing principal '"
<< (principal.isSome() ? principal.get() : "ANY")
<< "' to reserve resources";
```
src/master/master.cpp (line 2754)
<https://reviews.apache.org/r/39987/#comment165224>
We don't use tailing period for log messages. Please do a sweep to fix all of them.
src/master/master.cpp (lines 2782 - 2789)
<https://reviews.apache.org/r/39987/#comment165221>
I think we should assume validation::operation::validate(unreserve) has already been called. Maybe use a CHECK here is more appropriate.
```
CHECK(isDynamicallyReserved(resource))
<< ...
```
src/master/master.cpp (lines 2794 - 2799)
<https://reviews.apache.org/r/39987/#comment165222>
Can you just stringify(unreserve.resources()) below when calling LOG(INFO)?
src/master/master.cpp (lines 2802 - 2804)
<https://reviews.apache.org/r/39987/#comment165225>
Ditto above. No need for the 'principalString' temp variable.
- Jie Yu
On Nov. 13, 2015, 4:30 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2015, 4:30 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
> src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
>
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
>
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation before authorization is because we want to get all the principals from resources in UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization with validation does cause some confusion when generating error messages. How about the following:
>
> 1) we don't perform validation in authorization. In authorizeUnreserveResources, when we iterate all resources, we can do the following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
> if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
> }
> }
> ```
>
> 2) in Http::reserve and Http::unreserve, we still perform validation first before calling master->authroizeXXX.
>
> 3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
>
> Let me know if that's better or not?
No worries, Jie! I learned about `Future<>::repair()` in the process so I consider it worthwhile :-)
I have one question about your plan: for `LAUNCH` operations, tasks are authorized before they get validated, and I'm wondering if it's worth making this part of the interface consistent across all types of operations, i.e. perhaps we should change the code for `LAUNCH` to validate before authorization as well. If we decide to do this, I could do it here or in another review.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
>
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
>
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation before authorization is because we want to get all the principals from resources in UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization with validation does cause some confusion when generating error messages. How about the following:
>
> 1) we don't perform validation in authorization. In authorizeUnreserveResources, when we iterate all resources, we can do the following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
> if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
> }
> }
> ```
>
> 2) in Http::reserve and Http::unreserve, we still perform validation first before calling master->authroizeXXX.
>
> 3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
>
> Let me know if that's better or not?
>
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I consider it worthwhile :-)
>
> I have one question about your plan: for `LAUNCH` operations, tasks are authorized before they get validated, and I'm wondering if it's worth making this part of the interface consistent across all types of operations, i.e. perhaps we should change the code for `LAUNCH` to validate before authorization as well. If we decide to do this, I could do it here or in another review.
>
> Greg Mann wrote:
> Since the validation for `LAUNCH` needs to be aware of tasks that were previously launched on the same operation, it makes sense in that case for validation to occur after authorization. However, in general, I think it makes more sense for the operation to be validated before authorization, so I'll follow the pattern that Jie suggested above for `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have slightly different expectations regarding validation depending on what type of operation is being performed.
>
> An alternative would be to validate all operations *after* authorization is performed, which in general seems less intuitive to me, but would keep the interface consistent...
>
> Jie Yu wrote:
> Greg, the code sample I suggested above actually tried to keep the interface consistent (validation is done separately and is decoupled from authorization). Just want to double check with you.
>
> Greg Mann wrote:
> Thanks for checking Jie, I think I misread your comments previously.
>
> Your suggested solution would keep the separation of validation and authorization consistent, the difference is that for HTTP endpoint operations validation would be done *before* authorization, while for framework operations validation would be done *after* authorization. I guess the question is, do we think it's worthwhile to establish a guideline like "you can always assume an operation has been validated before the authorization function is called" or vice-versa. Maybe it's inconsequential, since as the code exists currently it doesn't really matter in what order the two operations are performed.
Aha, now I get it. Thanks for explaining! Yeah, I think I am fine with either way. You can swap the order in http endpoint if the change is easy. Otherwise, I would probably just leave it as it is.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
>
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
>
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation before authorization is because we want to get all the principals from resources in UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization with validation does cause some confusion when generating error messages. How about the following:
>
> 1) we don't perform validation in authorization. In authorizeUnreserveResources, when we iterate all resources, we can do the following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
> if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
> }
> }
> ```
>
> 2) in Http::reserve and Http::unreserve, we still perform validation first before calling master->authroizeXXX.
>
> 3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
>
> Let me know if that's better or not?
>
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I consider it worthwhile :-)
>
> I have one question about your plan: for `LAUNCH` operations, tasks are authorized before they get validated, and I'm wondering if it's worth making this part of the interface consistent across all types of operations, i.e. perhaps we should change the code for `LAUNCH` to validate before authorization as well. If we decide to do this, I could do it here or in another review.
>
> Greg Mann wrote:
> Since the validation for `LAUNCH` needs to be aware of tasks that were previously launched on the same operation, it makes sense in that case for validation to occur after authorization. However, in general, I think it makes more sense for the operation to be validated before authorization, so I'll follow the pattern that Jie suggested above for `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have slightly different expectations regarding validation depending on what type of operation is being performed.
>
> An alternative would be to validate all operations *after* authorization is performed, which in general seems less intuitive to me, but would keep the interface consistent...
Greg, the code sample I suggested above actually tried to keep the interface consistent (validation is done separately and is decoupled from authorization). Just want to double check with you.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
>
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
>
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation before authorization is because we want to get all the principals from resources in UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization with validation does cause some confusion when generating error messages. How about the following:
>
> 1) we don't perform validation in authorization. In authorizeUnreserveResources, when we iterate all resources, we can do the following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
> if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
> }
> }
> ```
>
> 2) in Http::reserve and Http::unreserve, we still perform validation first before calling master->authroizeXXX.
>
> 3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
>
> Let me know if that's better or not?
>
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I consider it worthwhile :-)
>
> I have one question about your plan: for `LAUNCH` operations, tasks are authorized before they get validated, and I'm wondering if it's worth making this part of the interface consistent across all types of operations, i.e. perhaps we should change the code for `LAUNCH` to validate before authorization as well. If we decide to do this, I could do it here or in another review.
>
> Greg Mann wrote:
> Since the validation for `LAUNCH` needs to be aware of tasks that were previously launched on the same operation, it makes sense in that case for validation to occur after authorization. However, in general, I think it makes more sense for the operation to be validated before authorization, so I'll follow the pattern that Jie suggested above for `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have slightly different expectations regarding validation depending on what type of operation is being performed.
>
> An alternative would be to validate all operations *after* authorization is performed, which in general seems less intuitive to me, but would keep the interface consistent...
>
> Jie Yu wrote:
> Greg, the code sample I suggested above actually tried to keep the interface consistent (validation is done separately and is decoupled from authorization). Just want to double check with you.
Thanks for checking Jie, I think I misread your comments previously.
Your suggested solution would keep the separation of validation and authorization consistent, the difference is that for HTTP endpoint operations validation would be done *before* authorization, while for framework operations validation would be done *after* authorization. I guess the question is, do we think it's worthwhile to establish a guideline like "you can always assume an operation has been validated before the authorization function is called" or vice-versa. Maybe it's inconsequential, since as the code exists currently it doesn't really matter in what order the two operations are performed.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
>
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
I looked at the code again. I guess the reason we validate the operation before authorization is because we want to get all the principals from resources in UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization with validation does cause some confusion when generating error messages. How about the following:
1) we don't perform validation in authorization. In authorizeUnreserveResources, when we iterate all resources, we can do the following as you previosly did (sorry about the back and forth, my bad).
```
foreach (const Resource& resource, unreserve.resources()) {
if (Resources::isDynamicallyReserved(resource)) {
request.mutable_reserver_principals()->add_values(
resource.reservation().principal());
}
}
```
2) in Http::reserve and Http::unreserve, we still perform validation first before calling master->authroizeXXX.
3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
Let me know if that's better or not?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> > Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
>
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked that way. We ended up switching to this solution because it makes for a cleaner implementation and eliminates some redundant code. However, it does come at the cost of making error messaging more difficult (as you noted in another patch) and decreasing the separation of functionality that can make debugging easier. I don't have a strong preference for either path at the moment. Perhaps Jie could chime in with his thoughts?
>
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation before authorization is because we want to get all the principals from resources in UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization with validation does cause some confusion when generating error messages. How about the following:
>
> 1) we don't perform validation in authorization. In authorizeUnreserveResources, when we iterate all resources, we can do the following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
> if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
> }
> }
> ```
>
> 2) in Http::reserve and Http::unreserve, we still perform validation first before calling master->authroizeXXX.
>
> 3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
>
> Let me know if that's better or not?
>
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I consider it worthwhile :-)
>
> I have one question about your plan: for `LAUNCH` operations, tasks are authorized before they get validated, and I'm wondering if it's worth making this part of the interface consistent across all types of operations, i.e. perhaps we should change the code for `LAUNCH` to validate before authorization as well. If we decide to do this, I could do it here or in another review.
Since the validation for `LAUNCH` needs to be aware of tasks that were previously launched on the same operation, it makes sense in that case for validation to occur after authorization. However, in general, I think it makes more sense for the operation to be validated before authorization, so I'll follow the pattern that Jie suggested above for `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have slightly different expectations regarding validation depending on what type of operation is being performed.
An alternative would be to validate all operations *after* authorization is performed, which in general seems less intuitive to me, but would keep the interface consistent...
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108496
-----------------------------------------------------------
src/master/master.hpp (line 706)
<https://reviews.apache.org/r/39987/#comment167947>
If the intention is to be similar to the `authorizeTask` interface, we should take `Resources` here instead? Although I think taking `Offer::Operation::Reserve` would be best here.
src/master/master.hpp (line 730)
<https://reviews.apache.org/r/39987/#comment167948>
Similar question as above.
src/master/master.cpp (line 2771)
<https://reviews.apache.org/r/39987/#comment167950>
Why is it that we need to perform validation within authorization? We perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later on. Does that pattern not work here?
- Michael Park
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108395
-----------------------------------------------------------
src/master/master.cpp (lines 2776 - 2778)
<https://reviews.apache.org/r/39987/#comment167848>
Let's not drop the operation here. Let the caller do it if Error is returned.
src/master/master.cpp (line 2780)
<https://reviews.apache.org/r/39987/#comment167853>
return Failure(error.get().message);
src/master/master.cpp (line 2810)
<https://reviews.apache.org/r/39987/#comment167855>
Instead of passing in the Framework pointer, can we pass in the optional role instead?
- Jie Yu
On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:35 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Dec. 2, 2015, 10:38 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Altered comment to match changes in subsequent patches.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Dec. 2, 2015, 7:52 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Addressed comment.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108697
-----------------------------------------------------------
src/master/master.cpp (lines 2804 - 2805)
<https://reviews.apache.org/r/39987/#comment168153>
Do you need add an if check here given that validation is not a pre-condition of this method
```
if (isDynamicallyReserved(resource)) {
...
}
```
- Jie Yu
On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 6 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Dec. 2, 2015, 6 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Addressed comments.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 2, 2015, 2:17 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 2775
> > <https://reviews.apache.org/r/39987/diff/10/?file=1150202#file1150202line2775>
> >
> > I believe you inherited the TODO from MPark. Do you want to preserve his name or change to yours?
I was going to leave his name, since he's the original author of the TODO :-)
> On Dec. 2, 2015, 2:17 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 2781
> > <https://reviews.apache.org/r/39987/diff/10/?file=1150202#file1150202line2781>
> >
> > Maybe "' to reserve any resources"? Or mention what resources are allowed to reserve (similar to what you do in `authorizeUnreserveResources`.
Good call! Changed.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108638
-----------------------------------------------------------
On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 6 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108638
-----------------------------------------------------------
A drive-by review, I'm looking into authz and ACLs and learning from your patches.
src/master/master.cpp (line 2775)
<https://reviews.apache.org/r/39987/#comment168095>
I believe you inherited the TODO from MPark. Do you want to preserve his name or change to yours?
src/master/master.cpp (line 2781)
<https://reviews.apache.org/r/39987/#comment168094>
Maybe "' to reserve any resources"? Or mention what resources are allowed to reserve (similar to what you do in `authorizeUnreserveResources`.
src/master/master.cpp (lines 2811 - 2812)
<https://reviews.apache.org/r/39987/#comment168096>
Fits one line (clang-format suggests the same).
- Alexander Rukletsov
On Dec. 2, 2015, 9 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 9 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 2, 2015, 4:02 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2769-2770
> > <https://reviews.apache.org/r/39987/diff/10/?file=1150202#file1150202line2769>
> >
> > Is my understanding is correct, we do not allow reserving resources neither for operators nor for frameworks without a principal. Why do we have a path for `ACL::Entity::Any` here then? If you plan to reserve it for future use cases or just be more general — it's fine, but let's leave a fat comment that this is not possible currently (or how it's possible if my understanding is not correct).
> >
> > A follow-up question: Is it something we have agreed and maybe even documented somewhere, that certain actions require principal? I'm thinking about authz for quota and whether we have to make `principal` required there.
>
> Greg Mann wrote:
> Yea this is an artifact of doing validation after authorization. We're doing it in that order here to maintain consistency with the order for LAUNCH operations, which must validate after authorization because their validation requires knowledge of the tasks that may have already been launched within a particular operation.
>
> You're right that if `!principal.isSome()` here, we know that this operation will fail. The validation routine will catch that and return the proper error, but how best to handle it here? In order to ensure that `validate()` is able to report the correct error, perhaps we should just `return true;` when `!principal.isSome()`. This is less than ideal, but perhaps would suffice along with a fat comment, as you suggested :-)
Alex, thank you for the comments! I think those are great questions.
> whether we have to make principal required there.
I like the idea of making it optional. That gives us more flexibility (see some discussion in MESOS-3064): allowing dynamic reserveration without authz.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108655
-----------------------------------------------------------
On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 6 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 2, 2015, 4:02 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2769-2770
> > <https://reviews.apache.org/r/39987/diff/10/?file=1150202#file1150202line2769>
> >
> > Is my understanding is correct, we do not allow reserving resources neither for operators nor for frameworks without a principal. Why do we have a path for `ACL::Entity::Any` here then? If you plan to reserve it for future use cases or just be more general — it's fine, but let's leave a fat comment that this is not possible currently (or how it's possible if my understanding is not correct).
> >
> > A follow-up question: Is it something we have agreed and maybe even documented somewhere, that certain actions require principal? I'm thinking about authz for quota and whether we have to make `principal` required there.
Yea this is an artifact of doing validation after authorization. We're doing it in that order here to maintain consistency with the order for LAUNCH operations, which must validate after authorization because their validation requires knowledge of the tasks that may have already been launched within a particular operation.
You're right that if `!principal.isSome()` here, we know that this operation will fail. The validation routine will catch that and return the proper error, but how best to handle it here? In order to ensure that `validate()` is able to report the correct error, perhaps we should just `return true;` when `!principal.isSome()`. This is less than ideal, but perhaps would suffice along with a fat comment, as you suggested :-)
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108655
-----------------------------------------------------------
On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 6 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review108655
-----------------------------------------------------------
src/master/master.cpp (lines 2769 - 2770)
<https://reviews.apache.org/r/39987/#comment168116>
Is my understanding is correct, we do not allow reserving resources neither for operators nor for frameworks without a principal. Why do we have a path for `ACL::Entity::Any` here then? If you plan to reserve it for future use cases or just be more general — it's fine, but let's leave a fat comment that this is not possible currently (or how it's possible if my understanding is not correct).
A follow-up question: Is it something we have agreed and maybe even documented somewhere, that certain actions require principal? I'm thinking about authz for quota and whether we have to make `principal` required there.
- Alexander Rukletsov
On Dec. 2, 2015, 9 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 9 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
> src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Dec. 2, 2015, 9 a.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Addressed comments.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Nov. 30, 2015, 8:35 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Address comments, refactor to include validation.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe
src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/#review107278
-----------------------------------------------------------
Ship it!
src/master/master.cpp (line 2792)
<https://reviews.apache.org/r/39987/#comment166308>
You don't need the 'stringify' here because '<<' operator overload will call stringify for you.
- Jie Yu
On Nov. 16, 2015, 11:48 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2015, 11:48 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
>
>
> Diffs
> -----
>
> src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80
> src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28
>
> Diff: https://reviews.apache.org/r/39987/diff/
>
>
> Testing
> -------
>
> This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Nov. 16, 2015, 11:48 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Rebase.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80
src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Nov. 13, 2015, 11:32 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Addressed comments: removed logging variables, etc.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Nov. 13, 2015, 11:15 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Updated ACL names.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 39987: [3/5] Added
'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39987/
-----------------------------------------------------------
(Updated Nov. 13, 2015, 4:30 a.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/
Diffs (updated)
-----
src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1
Diff: https://reviews.apache.org/r/39987/diff/
Testing
-------
This is the third in a chain of 5 patches. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann