You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/12/01 15:24:27 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/#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 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
>
>