> On 十二月 1, 2016, 10:36 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 2336-2349
> > <https://reviews.apache.org/r/54062/diff/6/?file=1572829#file1572829line2336>
> >
> >     It seems problematic here that `frameworkInfo.role()` is accesssed when 
> > the framework is MULTI_ROLE capable, since it returns `"*"` and will be 
> > checked against the whitelist (and this would fail if `"*"` is not in the 
> > whitelist!).
> >     
> >     You could do something like this:
> >     
> >     ```
> >     if (validationError.isNone()) {
> >       // Check the framework's role(s) against the whitelist.
> >       set<string> invalidRoles;
> >       
> >       if (frameworkHasCapability(frameworkInfo, MULTI_ROLE)) {
> >         foreach (const string& role, frameworkInfo.roles()) {
> >           if (!isWhitelisted(role)) {
> >             invalidRoles.insert(role);
> >           }
> >         }
> >       } else {
> >         if (!isWhitelisted(frameworkInfo.role())) {
> >           invalidRoles.insert(frameworkInfo.role());
> >         }
> >       }
> >       
> >       validationError = Error("Roles " + stringify(invalidRoles) +
> >                               " are not present in master's --roles");
> >     }
> >     ```

Hi Ben, we can make sure that there will always be a `*` role in the whitelist 
if operator setting `--roles` when master start up, please refer to 
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L665-L666 , 
so seems it is ok to ignore the `MULTI_ROLE` check here. 

But agree with you that here your suggestion is more clear as we can have all 
of the invalid roles be printed out and this is helpful for troubleshooting.


> On 十二月 1, 2016, 10:36 p.m., Benjamin Mahler wrote:
> > src/master/validation.hpp, lines 59-78
> > <https://reviews.apache.org/r/54062/diff/6/?file=1572830#file1572830line59>
> >
> >     Let's also mention here that duplicate role entries are not allowed?
> >     
> >     Also, how about using + characters for the intersecting lines? I send 
> > to see this more commonly, e.g.: https://ozh.github.io/ascii-tables/

As here we are also allowing frameworks with MULTI_ROLE capability can be 
registered **without** roles, and for this case, the framework will not get any 
resources, it is better that we also mention this case here?


- Guangya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54062/#review157651
-----------------------------------------------------------


On 十一月 30, 2016, 9:57 a.m., Jay Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> -----------------------------------------------------------
> 
> (Updated 十一月 30, 2016, 9:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang.
> 
> 
> Bugs: MESOS-6629
>     https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We need to do necessary validation for the conflicts of role, roles
> and MULTI_ROLE capability. It complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
>         |-------|---------|
>         |Roles  |No Roles |
> |-------|-------|---------|
> |Role   | Error |  None   |
> |-------|-------|---------|
> |No Role| Error |  None   |
> |-------|-------|---------|
> 
> --- MULTI_ROLE is set ----
>         |-------|---------|
>         |Roles  |No Roles |
> |-------|-------|---------|
> |Role   | Error |  Error  |
> |-------|-------|---------|
> |No Role| None  |  None   |
> |-------|-------|---------|
> 
> One test case is added to ensure validateRoles() catches all invalid
> combinations of these attributes. Another three test cases are added
> to ensure the master accepts/rejects subscription given valid/invalid
> multiple roles.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 
>   src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Reply via email to