mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 55462: Validate resource reservation against allocated role.
Date Fri, 10 Feb 2017 15:16:18 GMT


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


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