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