mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 55462: Validate resource reservation against allocated role.
Date Fri, 10 Feb 2017 23:55:43 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message