You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2017/01/12 14:54:14 UTC
Review Request 55462: WIP: Validate resource reservation against
allocated role.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Repository: mesos
Description
-------
This just adds possible modifications as comments. The changes can only be implemented once MESOS-6635 landed.
Diffs
-----
src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: WIP: Validate resource reservation against
allocated role.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review161388
-----------------------------------------------------------
Bad patch!
Reviews applied: [55462, 55461]
Failed command: python support/apply-reviews.py -n -r 55461
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 349, in <module>
reviewboard()
File "support/apply-reviews.py", line 328, in reviewboard
apply_review()
File "support/apply-reviews.py", line 121, in apply_review
fetch_patch()
File "support/apply-reviews.py", line 150, in fetch_patch
r = urllib2.urlopen(patch_url(), context=ssl_create_default_context())
File "support/apply-reviews.py", line 131, in ssl_create_default_context
context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
AttributeError: 'module' object has no attribute 'SSLContext'
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
File "support/apply-reviews.py", line 119, in <lambda>
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '55461.patch'
Error in sys.exitfunc:
Traceback (most recent call last):
File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
File "support/apply-reviews.py", line 119, in <lambda>
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '55461.patch'
Full log: https://builds.apache.org/job/Mesos-Reviewbot/16696/console
- Mesos ReviewBot
On Jan. 12, 2017, 2:54 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 2:54 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This just adds possible modifications as comments. The changes can only be implemented once MESOS-6635 landed.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 10, 2017, 2:39 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1623-1630
> > <https://reviews.apache.org/r/55462/diff/3/?file=1627596#file1627596line1623>
> >
> > Just to clarify, this is an invariant in that if this fails it is a programming error in the master, right? It seems to me we have two invariants here:
> >
> > (1) Coming from framework: framework info is set and resources are allocated (master enforces this when applying operations).
> >
> > (2) Coming from operator: framework info is not set and resources are unallocated (master doesn't enforce this).
> >
> > We should clarify this. Also, `!resource.allocation_info().has_role()` is sufficient here if you want to be more succinct.
>
> Benjamin Bannier wrote:
> I am not sure these cases are invariants *for this function*, and it seems us ensuring correct behavior with tests rather then fail hard here would decrease coupling between validation logic and apply operations and be just as good.
>
> The case (1) we do explicitly handle here seems interesting since it points to errors in framework code (e.g., incorrect handling of offered resources). Case (2) seems less likely as it would mean that the operator would set more information than needed or used by the master. In any case, every caller of the validation function would need to normalize the resulting operation in order to deal with the different possible inputs (e.g., (1) or (2) here).
>
> Does that make sense?
Yeah this was just to clarify we had the same understanding, and it sounds like we do. I'm also in favor of not failing hard here.
The other thing I would mention to be sure we both understand the same is that the operations won't apply later if case (1) or case (2) don't hold, even if we don't validate against these cases explicitly here.
I wonder if we should also be validating case (2) since we're validating case (1), since validating it here allows us to provide a more informative message. E.g. "A reserve operation was attempted on allocated resources, but operators can only reserve available resources". Feel free to add it in this patch or another patch.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165056
-----------------------------------------------------------
On Feb. 10, 2017, 3:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 3:16 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 10, 2017, 3:39 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1632-1634
> > <https://reviews.apache.org/r/55462/diff/3/?file=1627596#file1627596line1632>
> >
> > Shouldn't we also be checking that the allocation role matches the reservation role?
Yes, this seems rather crucial to this patch. Added a check for this, also a test.
> On Feb. 10, 2017, 3:39 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, line 1633
> > <https://reviews.apache.org/r/55462/diff/3/?file=1627596#file1627596line1633>
> >
> > Hm.. this case seems to warrant a different message?
Done.
> On Feb. 10, 2017, 3:39 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1623-1630
> > <https://reviews.apache.org/r/55462/diff/3/?file=1627596#file1627596line1623>
> >
> > Just to clarify, this is an invariant in that if this fails it is a programming error in the master, right? It seems to me we have two invariants here:
> >
> > (1) Coming from framework: framework info is set and resources are allocated (master enforces this when applying operations).
> >
> > (2) Coming from operator: framework info is not set and resources are unallocated (master doesn't enforce this).
> >
> > We should clarify this. Also, `!resource.allocation_info().has_role()` is sufficient here if you want to be more succinct.
I am not sure these cases are invariants *for this function*, and it seems us ensuring correct behavior with tests rather then fail hard here would decrease coupling between validation logic and apply operations and be just as good.
The case (1) we do explicitly handle here seems interesting since it points to errors in framework code (e.g., incorrect handling of offered resources). Case (2) seems less likely as it would mean that the operator would set more information than needed or used by the master. In any case, every caller of the validation function would need to normalize the resulting operation in order to deal with the different possible inputs (e.g., (1) or (2) here).
Does that make sense?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165056
-----------------------------------------------------------
On Feb. 10, 2017, 4:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 4:16 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165056
-----------------------------------------------------------
src/master/validation.cpp (lines 1623 - 1630)
<https://reviews.apache.org/r/55462/#comment236901>
Just to clarify, this is an invariant in that if this fails it is a programming error in the master, right? It seems to me we have two invariants here:
(1) Coming from framework: framework info is set and resources are allocated (master enforces this when applying operations).
(2) Coming from operator: framework info is not set and resources are unallocated (master doesn't enforce this).
We should clarify this. Also, `!resource.allocation_info().has_role()` is sufficient here if you want to be more succinct.
src/master/validation.cpp (lines 1632 - 1634)
<https://reviews.apache.org/r/55462/#comment236903>
Shouldn't we also be checking that the allocation role matches the reservation role?
src/master/validation.cpp (line 1633)
<https://reviews.apache.org/r/55462/#comment236897>
Hm.. this case seems to warrant a different message?
- Benjamin Mahler
On Feb. 8, 2017, 11:41 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 11:41 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 37d171512ee54f260aabb6e1071739bcc3769fb0
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 11, 2017, 12:01 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1612-1629
> > <https://reviews.apache.org/r/55462/diff/4/?file=1629533#file1629533line1612>
> >
> > Looks like this needs a rebase against master? This block was removed and the roles are no longer an option.
>
> Benjamin Bannier wrote:
> No, this patch was created against a recent `HEAD`. The modified patch you committed as part of r/55461 introduced a loop-local `set<string> frameworkRoles` inside the loop over resources. That is nice as it avoids keeping two optional, related values in scope (`frameworkInfo` and `frameworkRoles`). This comes at the cost of recalculating this framework invariant for each loop iteration; I here pulled it out of the loop to not recalculate it over and over again. Do you believe that's a bad idea?
Ok that makes sense. And what about the validation that is in this block? Based on the discussion from the last review these were removed, are you suggesting to add them back in?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165204
-----------------------------------------------------------
On Feb. 13, 2017, 3:38 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 3:38 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 11, 2017, 1:01 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1612-1629
> > <https://reviews.apache.org/r/55462/diff/4/?file=1629533#file1629533line1612>
> >
> > Looks like this needs a rebase against master? This block was removed and the roles are no longer an option.
No, this patch was created against a recent `HEAD`. The modified patch you committed as part of r/55461 introduced a loop-local `set<string> frameworkRoles` inside the loop over resources. That is nice as it avoids keeping two optional, related values in scope (`frameworkInfo` and `frameworkRoles`). This comes at the cost of recalculating this framework invariant for each loop iteration; I here pulled it out of the loop to not recalculate it over and over again. Do you believe that's a bad idea?
> On Feb. 11, 2017, 1:01 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1659-1695
> > <https://reviews.apache.org/r/55462/diff/4/?file=1629533#file1629533line1659>
> >
> > See my response to the thread in the previous review comments about validating case (2) as well so that we can provide a better error message to the operators.
Updated.
> On Feb. 11, 2017, 1:01 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1667-1668
> > <https://reviews.apache.org/r/55462/diff/4/?file=1629533#file1629533line1667>
> >
> > How about:
> >
> > A reserve operation was attempted on unallocated resources, but frameworks can only perform reservations on allocated resources
> >
> > Or:
> >
> > A reserve operation was attempted on unallocated resource 'XXX', but frameworks can only perform reservations on allocated resources
Went for opt 2.
> On Feb. 11, 2017, 1:01 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1665-1693
> > <https://reviews.apache.org/r/55462/diff/4/?file=1629533#file1629533line1665>
> >
> > Is it possible to get the open and close quotes on the same lines for these error messages? When on different lines we tend to forget to close them, e.g.
> >
> > ```
> > return Error(
> > "A reserve operation was attempted for a resource allocated to role"
> > " '" + resource.role() + "'"
> > ", but the framework only has roles"
> > " '" + stringify(frameworkRoles.get()) + "'");
> > ```
Done.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165204
-----------------------------------------------------------
On Feb. 13, 2017, 4:38 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 4:38 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165204
-----------------------------------------------------------
src/master/validation.cpp (lines 1612 - 1629)
<https://reviews.apache.org/r/55462/#comment237018>
Looks like this needs a rebase against master? This block was removed and the roles are no longer an option.
src/master/validation.cpp (lines 1659 - 1695)
<https://reviews.apache.org/r/55462/#comment237023>
See my response to the thread in the previous review comments about validating case (2) as well so that we can provide a better error message to the operators.
src/master/validation.cpp (lines 1665 - 1693)
<https://reviews.apache.org/r/55462/#comment237025>
Is it possible to get the open and close quotes on the same lines for these error messages? When on different lines we tend to forget to close them, e.g.
```
return Error(
"A reserve operation was attempted for a resource allocated to role"
" '" + resource.role() + "'"
", but the framework only has roles"
" '" + stringify(frameworkRoles.get()) + "'");
```
src/master/validation.cpp (lines 1667 - 1668)
<https://reviews.apache.org/r/55462/#comment237024>
How about:
A reserve operation was attempted on unallocated resources, but frameworks can only perform reservations on allocated resources
Or:
A reserve operation was attempted on unallocated resource 'XXX', but frameworks can only perform reservations on allocated resources
src/tests/master_validation_tests.cpp (lines 420 - 446)
<https://reviews.apache.org/r/55462/#comment237026>
Looks good, did you want to test the other cases as well?
- Benjamin Mahler
On Feb. 10, 2017, 3:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 3:16 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 14, 2017, 1:11 a.m., Benjamin Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 471-490
> > <https://reviews.apache.org/r/55462/diff/5/?file=1631983#file1631983line471>
> >
> > We could generalize this test to be: a framework (that has role `"*"` in its roles, e.g. `{role1, role2, *}`) cannot make a reservation to `*`. As it stands we seem to be missing the case where star is present but there are multiple roles.
There is some needed redundancy as the error messages for the different cases are not identical, and checking these helps avoid randomly catching any other validation we might introduce on the data hardcoded in this test at some later point.
I was also thinking about `[*, ..]`, but ultimately decided against adding an additional check for that since it didn't really cover new branches in the originally proposed validation code; this seems even more true now where we do not explicitly handle an empty `roles` set or `role=*` anymore in the validation. Instead the code uses some general set of roles and this test would fail would that validation be e.g., removed. Do you think it would still make sense to add such a test for added redundancy?
> On Feb. 14, 2017, 1:11 a.m., Benjamin Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 493-513
> > <https://reviews.apache.org/r/55462/diff/5/?file=1631983#file1631983line493>
> >
> > This could be generalized to be making a reservation to a role that is not within its roles (which may be already caught by the mismatched allocation role since we expect allocation role == reserve role)?
I added test cases for default role, empty roles, and some values for role or roles, and merged this with the previous test case. I made these inline, but can work to generate these instead (requires some extra legwork to generate differing error message which I believe would be worth it). Let me know having these generated instead of inlined makes more sense to you.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165404
-----------------------------------------------------------
On Feb. 14, 2017, 2:47 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 2:47 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 14, 2017, 1:11 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1617-1629
> > <https://reviews.apache.org/r/55462/diff/5/?file=1631982#file1631982line1617>
> >
> > We should be able to just remove these per the discussion from the last review.
Done per discussion from https://reviews.apache.org/r/55461/.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165404
-----------------------------------------------------------
On Feb. 14, 2017, 1:30 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 1:30 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165404
-----------------------------------------------------------
src/master/validation.cpp (lines 1617 - 1629)
<https://reviews.apache.org/r/55462/#comment237238>
We should be able to just remove these per the discussion from the last review.
src/master/validation.cpp (line 1668)
<https://reviews.apache.org/r/55462/#comment237242>
s/+s/+ s/
src/master/validation.cpp (lines 1668 - 1670)
<https://reviews.apache.org/r/55462/#comment237243>
Hm.. the ", but" line seems a bit odd, how about:
```
return Error(
"A reserved operation was attempted on unallocated resource"
" '" +stringify(resource) + "', but frameworks can only"
" perform reservations on allocated resources");
```
src/master/validation.cpp (lines 1675 - 1677)
<https://reviews.apache.org/r/55462/#comment237244>
Is it possible to get the quotes on the same line? E.g.
```
return Error(
"A reserve operation was attempted for a resource with role"
" '" + resource.role() + "', but the resource was allocated"
" to role '" + resource.allocation_info().role() + "'");
```
src/master/validation.cpp (lines 1682 - 1686)
<https://reviews.apache.org/r/55462/#comment237237>
Is it possible to put the quotes on the same line?
```
return Error(
"A reserve operation was attempted for a resource allocated to role"
" '" + resource.role() + "', but the framework only has roles"
" '" + stringify(frameworkRoles.get()) + "'");
```
src/master/validation.cpp (line 1694)
<https://reviews.apache.org/r/55462/#comment237245>
s/"' "/" '"/
src/master/validation.cpp (lines 1698 - 1699)
<https://reviews.apache.org/r/55462/#comment237239>
For a bit more clarity: "set because operators can only reserve the available resources"
src/master/validation.cpp (lines 1702 - 1703)
<https://reviews.apache.org/r/55462/#comment237246>
Hm.. what does "out-of-band" allocation info mean?
How about something like the following, which makes it clear what the operator API allows:
```
A reserve operation was attempted with an allocated resource but the operator API only allows reservations
to be made to the unallocated resources
```
src/tests/master_validation_tests.cpp (lines 424 - 425)
<https://reviews.apache.org/r/55462/#comment237247>
How about having this block right next to the reserve block since these two seem related, seems a bit clearer?
src/tests/master_validation_tests.cpp (lines 471 - 490)
<https://reviews.apache.org/r/55462/#comment237248>
We could generalize this test to be: a framework (that has role `"*"` in its roles, e.g. `{role1, role2, *}`) cannot make a reservation to `*`. As it stands we seem to be missing the case where star is present but there are multiple roles.
src/tests/master_validation_tests.cpp (lines 493 - 513)
<https://reviews.apache.org/r/55462/#comment237250>
This could be generalized to be making a reservation to a role that is not within its roles (which may be already caught by the mismatched allocation role since we expect allocation role == reserve role)?
- Benjamin Mahler
On Feb. 13, 2017, 3:38 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 3:38 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/#review165796
-----------------------------------------------------------
Fix it, then Ship it!
Looks like some of the comments from the previous review were missed? Also, I simplified some of the tests.
src/tests/master_validation_tests.cpp (lines 472 - 474)
<https://reviews.apache.org/r/55462/#comment237649>
This test seems like it should just be part of the 'MatchingRole' test?
- Benjamin Mahler
On Feb. 14, 2017, 1:47 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 1:47 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
> src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
>
> Diff: https://reviews.apache.org/r/55462/diff/
>
>
> Testing
> -------
>
> N/A yet.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 2:47 a.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Anglicize error message, streamlined tests.
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description
-------
This change introduces validation of the 'AllocationInfo' of resources
used in reservations.
Diffs (updated)
-----
src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 1:30 a.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Removed redundant validation, update test.
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description
-------
This change introduces validation of the 'AllocationInfo' of resources
used in reservations.
Diffs (updated)
-----
src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Feb. 13, 2017, 4:38 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Addressed comments from bmahler:
* improved error messages
* increased test coverage
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description
-------
This change introduces validation of the 'AllocationInfo' of resources
used in reservations.
Diffs (updated)
-----
src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Feb. 10, 2017, 4:16 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Added missing validation, include test.
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description
-------
This change introduces validation of the 'AllocationInfo' of resources
used in reservations.
Diffs (updated)
-----
src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e
src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 12:41 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description
-------
This change introduces validation of the 'AllocationInfo' of resources
used in reservations.
Diffs (updated)
-----
src/master/validation.cpp 37d171512ee54f260aabb6e1071739bcc3769fb0
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 5:43 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Used `AllocationInfo` now in master.
Summary (updated)
-----------------
Validate resource reservation against allocated role.
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description (updated)
-------
This change introduces validation of the 'AllocationInfo' of resources
used in reservations.
Diffs (updated)
-----
src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier
Re: Review Request 55462: WIP: Validate resource reservation against
allocated role.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55462/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 8:10 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Bugs: MESOS-6730
https://issues.apache.org/jira/browse/MESOS-6730
Repository: mesos
Description
-------
This just adds possible modifications as comments. The changes can only be implemented once MESOS-6635 landed.
Diffs
-----
src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
Diff: https://reviews.apache.org/r/55462/diff/
Testing
-------
N/A yet.
Thanks,
Benjamin Bannier