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/06 19:07:50 UTC

Review Request 55271: Disallow multi-role frameworks to change their roles.

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

Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs
-----

  src/master/master.cpp 11c34a048586d30c6ac67be8638ed8fa81cc3f1f 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Guangya Liu <gy...@gmail.com>.

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



You may also want a test case in https://github.com/apache/mesos/blob/master/src/tests/master_validation_tests.cpp


src/master/master.cpp (lines 2400 - 2418)
<https://reviews.apache.org/r/55271/#comment232041>

    I think that you may also want to apply this to https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2535 
    
    ```
    void Master::subscribe(
        const UPID& from,
        const scheduler::Call::Subscribe& subscribe)
    ```


- Guangya Liu


On \u4e00\u6708 6, 2017, 7:07 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated \u4e00\u6708 6, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 11c34a048586d30c6ac67be8638ed8fa81cc3f1f 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55381, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 11, 2017, 10:37 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2017, 10:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Jay Guo <gu...@gmail.com>.


> On Jan. 17, 2017, 5:10 p.m., Jay Guo wrote:
> > Also, should we allow user to downgrade from a multi-role framework to single-role? I feel it would be very complicated and we should explicitly disallow that...
> 
> Benjamin Bannier wrote:
>     I am not sure this is required. We already make sure elsewhere that (1) only one of `role` or `roles` can be set, and (2) we also make sure that multirole frameworks cannot change their roles. Are there any implications of a framework opting in and out of `MULTI_ROLE`? In either case it would be in at most one role (some role or `*`). I have the feeling that adding such a validation might make experimenting with this feature harder than needed (if there are no technical reasons to explicitly forbid it). Any ideas @bmahler?

I was thinking that in a longer run, is it a valid use case that user wants to downgrade framework with `roles == {role1, role2}` to `role == role1`?


- Jay


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


On Jan. 18, 2017, 6:20 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2017, 6:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 17, 2017, 10:10 a.m., Jay Guo wrote:
> > Also, should we allow user to downgrade from a multi-role framework to single-role? I feel it would be very complicated and we should explicitly disallow that...
> 
> Benjamin Bannier wrote:
>     I am not sure this is required. We already make sure elsewhere that (1) only one of `role` or `roles` can be set, and (2) we also make sure that multirole frameworks cannot change their roles. Are there any implications of a framework opting in and out of `MULTI_ROLE`? In either case it would be in at most one role (some role or `*`). I have the feeling that adding such a validation might make experimenting with this feature harder than needed (if there are no technical reasons to explicitly forbid it). Any ideas @bmahler?
> 
> Jay Guo wrote:
>     I was thinking that in a longer run, is it a valid use case that user wants to downgrade framework with `roles == {role1, role2}` to `role == role1`?

@guoger: I believe at least for now going from `roles = {A, B}` to `role = A` should be just as illegal as going from `roles = {A, B}` to `roles = {A}`, I'll update the validation code to catch that cases as well. We should keep this kind of framework change path in mind for Phase 2, where I believe we would mutations of `roles`.


- Benjamin


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


On Jan. 17, 2017, 11:20 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 17, 2017, 10:10 a.m., Jay Guo wrote:
> > Also, should we allow user to downgrade from a multi-role framework to single-role? I feel it would be very complicated and we should explicitly disallow that...

I am not sure this is required. We already make sure elsewhere that (1) only one of `role` or `roles` can be set, and (2) we also make sure that multirole frameworks cannot change their roles. Are there any implications of a framework opting in and out of `MULTI_ROLE`? In either case it would be in at most one role (some role or `*`). I have the feeling that adding such a validation might make experimenting with this feature harder than needed (if there are no technical reasons to explicitly forbid it). Any ideas @bmahler?


> On Jan. 17, 2017, 10:10 a.m., Jay Guo wrote:
> > src/master/master.hpp, lines 2522-2525
> > <https://reviews.apache.org/r/55271/diff/7/?file=1605758#file1605758line2522>
> >
> >     This warning is not always true in case framework is being upgraded from single-role to multi-role.

I updated the patch so this warning is only emitted for non-multirole frameworks. Multirole frameworks should already be handled by the validation I added to the start of the function.


- Benjamin


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


On Jan. 17, 2017, 11:20 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Jay Guo <gu...@gmail.com>.

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



Also, should we allow user to downgrade from a multi-role framework to single-role? I feel it would be very complicated and we should explicitly disallow that...


src/master/master.hpp (lines 2522 - 2525)
<https://reviews.apache.org/r/55271/#comment233129>

    This warning is not always true in case framework is being upgraded from single-role to multi-role.


- Jay Guo


On Jan. 16, 2017, 7:28 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2017, 7:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55381, 55571, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos ReviewBot


On Jan. 17, 2017, 5:14 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2017, 5:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 21, 2017, 4:30 a.m., Michael Park wrote:
> > src/master/master.hpp, lines 2475-2516
> > <https://reviews.apache.org/r/55271/diff/13/?file=1610797#file1610797line2475>
> >
> >     We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop?
> >     
> >     It looks like this could be:
> >     
> >     ```cpp
> >     if (protobuf::frameworkHasCapability(
> >             info, FrameworkInfo::Capability::MULTI_ROLE) ||
> >         protobuf::frameworkHasCapability(
> >             source, FrameworkInfo::Capability::MULTI_ROLE)) {
> >       // ...
> >       
> >       if (oldRoles != newRoles) {
> >         return Error(...);
> >       }
> >     }
> >     ```

This was supposed to introduce a pattern for when more validation branches would be added; agree it is overkill ATM, and potentially not even that useful in the future, removed.


> On Jan. 21, 2017, 4:30 a.m., Michael Park wrote:
> > src/tests/master_validation_tests.cpp, lines 2718-2721
> > <https://reviews.apache.org/r/55271/diff/13/?file=1610798#file1610798line2718>
> >
> >     This is quite brittle, not only because the error message could change, but because we print out a `hashset`, the ordering is not deterministic...
> >     
> >     I might actually suggest using `std::set` so that we get alphabetical ordering of the role names which I think would be easier to read/compare in an error message.

Agreed, even if the ordering with `hashset` would not depend on the setup and be guaranteed identical ordering, the ordering produce by a default `set` is more natural.


> On Jan. 21, 2017, 4:30 a.m., Michael Park wrote:
> > src/tests/master_validation_tests.cpp, lines 2723-2724
> > <https://reviews.apache.org/r/55271/diff/13/?file=1610798#file1610798line2723>
> >
> >     Shift indentation to the left, here and below.

Done, also below.


> On Jan. 21, 2017, 4:30 a.m., Michael Park wrote:
> > src/master/master.hpp, line 2481
> > <https://reviews.apache.org/r/55271/diff/13/?file=1610797#file1610797line2481>
> >
> >     We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop?
> >     
> >     It looks like this could be:
> >     
> >     ```cpp
> >     if (protobuf::frameworkHasCapability(
> >             info, FrameworkInfo::Capability::MULTI_ROLE) ||
> >         protobuf::frameworkHasCapability(
> >             source, FrameworkInfo::Capability::MULTI_ROLE)) {
> >       // ...
> >       
> >       if (oldRoles != newRoles) {
> >         return Error(...);
> >       }
> >     }
> >     ```

Dropping as this duplicates another issue (see above).


- Benjamin


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


On Jan. 21, 2017, 1:39 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2017, 1:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Michael Park <mp...@apache.org>.


> On Jan. 20, 2017, 7:30 p.m., Michael Park wrote:
> > src/master/master.hpp, lines 2475-2516
> > <https://reviews.apache.org/r/55271/diff/13/?file=1610797#file1610797line2475>
> >
> >     We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop?
> >     
> >     It looks like this could be:
> >     
> >     ```cpp
> >     if (protobuf::frameworkHasCapability(
> >             info, FrameworkInfo::Capability::MULTI_ROLE) ||
> >         protobuf::frameworkHasCapability(
> >             source, FrameworkInfo::Capability::MULTI_ROLE)) {
> >       // ...
> >       
> >       if (oldRoles != newRoles) {
> >         return Error(...);
> >       }
> >     }
> >     ```
> 
> Benjamin Bannier wrote:
>     This was supposed to introduce a pattern for when more validation branches would be added; agree it is overkill ATM, and potentially not even that useful in the future, removed.

By `return Error(...);` I was also suggesting to remove `Option<Error> validationError = None();` entirely.


- Michael


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


On Jan. 21, 2017, 4:39 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2017, 4:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Michael Park <mp...@apache.org>.

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




src/master/master.hpp (lines 2475 - 2516)
<https://reviews.apache.org/r/55271/#comment233843>

    We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop?
    
    It looks like this could be:
    
    ```cpp
    if (protobuf::frameworkHasCapability(
            info, FrameworkInfo::Capability::MULTI_ROLE) ||
        protobuf::frameworkHasCapability(
            source, FrameworkInfo::Capability::MULTI_ROLE)) {
      // ...
      
      if (oldRoles != newRoles) {
        return Error(...);
      }
    }
    ```



src/master/master.hpp (line 2481)
<https://reviews.apache.org/r/55271/#comment233841>

    We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop?
    
    It looks like this could be:
    
    ```cpp
    if (protobuf::frameworkHasCapability(
            info, FrameworkInfo::Capability::MULTI_ROLE) ||
        protobuf::frameworkHasCapability(
            source, FrameworkInfo::Capability::MULTI_ROLE)) {
      // ...
      
      if (oldRoles != newRoles) {
        return Error(...);
      }
    }
    ```



src/tests/master_validation_tests.cpp (lines 2718 - 2721)
<https://reviews.apache.org/r/55271/#comment233846>

    This is quite brittle, not only because the error message could change, but because we print out a `hashset`, the ordering is not deterministic...
    
    I might actually suggest using `std::set` so that we get alphabetical ordering of the role names which I think would be easier to read/compare in an error message.



src/tests/master_validation_tests.cpp (lines 2723 - 2724)
<https://reviews.apache.org/r/55271/#comment233845>

    Shift indentation to the left, here and below.


- Michael Park


On Jan. 20, 2017, 9:53 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2017, 9:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55381, 55571, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Jan. 21, 2017, 12:39 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2017, 12:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Michael Park <mp...@apache.org>.


> On Jan. 24, 2017, 3:32 p.m., Michael Park wrote:
> > src/master/master.hpp, lines 2490-2504
> > <https://reviews.apache.org/r/55271/diff/15/?file=1612323#file1612323line2490>
> >
> >     What do you think about pulling the roles retrieval out?
> >     
> >     ```cpp
> >     auto getRoles = [](const FrameworkInfo& info) -> std::set<std::string> {
> >       if (protobuf::frameworkHasCapability(
> >               info, FrameworkInfo::Capability::MULTI_ROLE)) {
> >         return {info.roles().begin(), info.roles().end())};
> >         
> >       } else {
> >         return {info.role()};
> >       }
> >     };
> >     
> >     if (getRoles(info) != getRoles(source)) {
> >       return Error(...);
> >     }
> >     ```
> 
> Benjamin Bannier wrote:
>     I like this pattern. It might not be much shorter, but only contains the logic once, and we could use more "normal" control flow.
>     
>     I would prefer to not use a lambda here though as certain compilers might incorrectly cause ODR violations for some code involving lambdas in functions defined in headers (not sure it is actually happening here, and we don't seem to follow a dedicated pattern to avoid this). As ODR might invoke UB I'd prefer to just copy the code :/

I'm fine dropping this. I don't believe that we actually run into the lambda in header issue here since we don't actually pass the lambda to another function, but I think the right thing to do here is consider introducing a set of helpers for `FrameworkInfo`, where `getRoles` could be one of the candidates.


- Michael


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


On Jan. 24, 2017, 4:41 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2017, 4:41 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 4bd01fd21820b7cdb6c2a23436d50e00fbbd5849 
>   src/tests/master_validation_tests.cpp ce10ea4502d0a13faca28d288dd16cd5cb864f6e 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 25, 2017, 12:32 a.m., Michael Park wrote:
> > src/master/master.hpp, lines 2490-2504
> > <https://reviews.apache.org/r/55271/diff/15/?file=1612323#file1612323line2490>
> >
> >     What do you think about pulling the roles retrieval out?
> >     
> >     ```cpp
> >     auto getRoles = [](const FrameworkInfo& info) -> std::set<std::string> {
> >       if (protobuf::frameworkHasCapability(
> >               info, FrameworkInfo::Capability::MULTI_ROLE)) {
> >         return {info.roles().begin(), info.roles().end())};
> >         
> >       } else {
> >         return {info.role()};
> >       }
> >     };
> >     
> >     if (getRoles(info) != getRoles(source)) {
> >       return Error(...);
> >     }
> >     ```

I like this pattern. It might not be much shorter, but only contains the logic once, and we could use more "normal" control flow.

I would prefer to not use a lambda here though as certain compilers might incorrectly cause ODR violations for some code involving lambdas in functions defined in headers (not sure it is actually happening here, and we don't seem to follow a dedicated pattern to avoid this). As ODR might invoke UB I'd prefer to just copy the code :/


- Benjamin


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


On Jan. 25, 2017, 1:41 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2017, 1:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 4bd01fd21820b7cdb6c2a23436d50e00fbbd5849 
>   src/tests/master_validation_tests.cpp ce10ea4502d0a13faca28d288dd16cd5cb864f6e 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Michael Park <mp...@apache.org>.

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


Fix it, then Ship it!





src/master/master.hpp (lines 2490 - 2504)
<https://reviews.apache.org/r/55271/#comment234245>

    What do you think about pulling the roles retrieval out?
    
    ```cpp
    auto getRoles = [](const FrameworkInfo& info) -> std::set<std::string> {
      if (protobuf::frameworkHasCapability(
              info, FrameworkInfo::Capability::MULTI_ROLE)) {
        return {info.roles().begin(), info.roles().end())};
        
      } else {
        return {info.role()};
      }
    };
    
    if (getRoles(info) != getRoles(source)) {
      return Error(...);
    }
    ```



src/master/master.hpp (lines 2531 - 2535)
<https://reviews.apache.org/r/55271/#comment234247>

    What do you think about putting this as the `else` condition for
    
    ```cpp
        if (protobuf::frameworkHasCapability(
              info, FrameworkInfo::Capability::MULTI_ROLE) ||
            protobuf::frameworkHasCapability(
              source, FrameworkInfo::Capability::MULTI_ROLE))
    ```
    
    as opposed to spelling out the opposite condition?


- Michael Park


On Jan. 23, 2017, midnight, Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2017, midnight)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55381, 55571, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Jan. 23, 2017, 8 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2017, 8 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 25, 2017, 8:33 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 4bd01fd21820b7cdb6c2a23436d50e00fbbd5849 
  src/tests/master_validation_tests.cpp ce10ea4502d0a13faca28d288dd16cd5cb864f6e 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 25, 2017, 1:41 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.


Changes
-------

* addressed comment from mcypark
* renamed tests


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 4bd01fd21820b7cdb6c2a23436d50e00fbbd5849 
  src/tests/master_validation_tests.cpp ce10ea4502d0a13faca28d288dd16cd5cb864f6e 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 23, 2017, 9 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.


Changes
-------

Addressed comment from mpark.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
  src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 21, 2017, 1:39 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.


Changes
-------

Addressed comments by mpark.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
  src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 20, 2017, 6:53 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
  src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 19, 2017, 2:01 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
  src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55381, 55571, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Jan. 18, 2017, 7:07 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Jay Guo <gu...@gmail.com>.

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




src/tests/master_validation_tests.cpp (lines 2774 - 2776)
<https://reviews.apache.org/r/55271/#comment233592>

    s/One failover/On failover/
    s/and then from the agent/and then from the framework/


- Jay Guo


On Jan. 19, 2017, 3:07 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2017, 3:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 18, 2017, 8:07 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Added another test for roles change on master failover. Suggested by guoger here: https://reviews.apache.org/r/55571/#comment233128.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
  src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 18, 2017, 4:25 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Addressed comments from guoger.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description (updated)
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
  src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 18, 2017, 3:36 a.m., Jay Guo wrote:
> > src/master/master.hpp, lines 2522-2528
> > <https://reviews.apache.org/r/55271/diff/9/?file=1606938#file1606938line2522>
> >
> >     This check is only valid **iff** both `src` and `dest` frameworkInfo are single role, isn't it? maybe
> >     ```cpp
> >     if (!capabilities.multiRole &&
> >         !protobuf::frameworkHasCapability(
> >             source, FrameworkInfo::Capability::MULTI_ROLE) &&
> >         source.role() != info.role()) {
> >       ...
> >     }

Also updated the comment. I did use `protobuf::frameworkHasCapability` in both cases for uniformity.


- Benjamin


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


On Jan. 17, 2017, 11:20 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Jay Guo <gu...@gmail.com>.

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




src/master/master.hpp (lines 2522 - 2528)
<https://reviews.apache.org/r/55271/#comment233285>

    This check is only valid **iff** both `src` and `dest` frameworkInfo are single role, isn't it? maybe
    ```cpp
    if (!capabilities.multiRole &&
        !protobuf::frameworkHasCapability(
            source, FrameworkInfo::Capability::MULTI_ROLE) &&
        source.role() != info.role()) {
      ...
    }


- Jay Guo


On Jan. 18, 2017, 6:20 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2017, 6:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 17, 2017, 11:20 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Addressed a comment from guoger.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 17, 2017, 6:14 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Rebased.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55381, 55571, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos ReviewBot


On Jan. 16, 2017, 11:28 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2017, 11:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 16, 2017, 12:28 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Addressed comment from guoger: moved validation to `updateFrameworkInfo`.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 16, 2017, 10:54 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Addressed some comments from guoger and gyliu.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.cpp b863ff6e93931c3d1ee056248084c7f44caf2fd9 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 14, 2017, 10:04 p.m., Guangya Liu wrote:
> > src/tests/master_validation_tests.cpp, line 2616
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603889#file1603889line2616>
> >
> >     How about s/RejectRolesChange/RejectRolesChangeWithMutiRole
> 
> Benjamin Bannier wrote:
>     When I named the test `RejectRolesChange` instead of `RejectRoleChange` I was hoping it would already be clear that this test would deal with a multi-role scenario. Was that too subtle? I would like to avoid excessive redundancy in the test name.
> 
> Michael Park wrote:
>     How about `RejectMultiRolesChange`?
> 
> Benjamin Bannier wrote:
>     Isn't this also highly redundant?
>     
>     I believe going forward we should parameterize these tests to take either some initial and final `role` or `roles`, and an boolean for whether we expect this migration to pass. In that cases it would make even sense to name these tests `RolesChange` and `RolesChangeOnMasterFailover`. I am still unsure on what to do with the `s` in there.

To settle this, I am going with `RejectRoleChangeWithMultiRole` now. I feel this might be verbose, but less confusing than `RejectMultiRolesChange` (`MULTI_ROLE` vs. roles).


- Benjamin


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


On Jan. 25, 2017, 1:41 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2017, 1:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 4bd01fd21820b7cdb6c2a23436d50e00fbbd5849 
>   src/tests/master_validation_tests.cpp ce10ea4502d0a13faca28d288dd16cd5cb864f6e 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Michael Park <mp...@apache.org>.


> On Jan. 14, 2017, 1:04 p.m., Guangya Liu wrote:
> > src/tests/master_validation_tests.cpp, line 2616
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603889#file1603889line2616>
> >
> >     How about s/RejectRolesChange/RejectRolesChangeWithMutiRole
> 
> Benjamin Bannier wrote:
>     When I named the test `RejectRolesChange` instead of `RejectRoleChange` I was hoping it would already be clear that this test would deal with a multi-role scenario. Was that too subtle? I would like to avoid excessive redundancy in the test name.

How about `RejectMultiRolesChange`?


- Michael


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


On Jan. 23, 2017, midnight, Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2017, midnight)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 14, 2017, 10:04 p.m., Guangya Liu wrote:
> > src/tests/master_validation_tests.cpp, line 2616
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603889#file1603889line2616>
> >
> >     How about s/RejectRolesChange/RejectRolesChangeWithMutiRole
> 
> Benjamin Bannier wrote:
>     When I named the test `RejectRolesChange` instead of `RejectRoleChange` I was hoping it would already be clear that this test would deal with a multi-role scenario. Was that too subtle? I would like to avoid excessive redundancy in the test name.
> 
> Michael Park wrote:
>     How about `RejectMultiRolesChange`?

Isn't this also highly redundant?

I believe going forward we should parameterize these tests to take either some initial and final `role` or `roles`, and an boolean for whether we expect this migration to pass. In that cases it would make even sense to name these tests `RolesChange` and `RolesChangeOnMasterFailover`. I am still unsure on what to do with the `s` in there.


- Benjamin


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


On Jan. 23, 2017, 9 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2017, 9 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 14, 2017, 10:04 p.m., Guangya Liu wrote:
> > src/master/master.cpp, line 2490
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603888#file1603888line2490>
> >
> >     I think that you may want to 
> >     ```
> >     #include <stout/hashset.hpp>
> >     ```

This is already added as part of this patch.


> On Jan. 14, 2017, 10:04 p.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 2473-2506
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603888#file1603888line2473>
> >
> >     Can you move this right after 2448 to group the logic related with role?

Done, also for `Master::subscribe(const UPID&, const scheduler::Call::Subscribe&)`.


> On Jan. 14, 2017, 10:04 p.m., Guangya Liu wrote:
> > src/tests/master_validation_tests.cpp, line 2616
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603889#file1603889line2616>
> >
> >     How about s/RejectRolesChange/RejectRolesChangeWithMutiRole

When I named the test `RejectRolesChange` instead of `RejectRoleChange` I was hoping it would already be clear that this test would deal with a multi-role scenario. Was that too subtle? I would like to avoid excessive redundancy in the test name.


- Benjamin


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


On Jan. 12, 2017, 4:32 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2017, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Guangya Liu <gy...@gmail.com>.

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




src/master/master.cpp (lines 2473 - 2506)
<https://reviews.apache.org/r/55271/#comment232920>

    Can you move this right after 2448 to group the logic related with role?



src/master/master.cpp (line 2490)
<https://reviews.apache.org/r/55271/#comment232921>

    I think that you may want to 
    ```
    #include <stout/hashset.hpp>
    ```



src/tests/master_validation_tests.cpp (line 2616)
<https://reviews.apache.org/r/55271/#comment232922>

    How about s/RejectRolesChange/RejectRolesChangeWithMutiRole


- Guangya Liu


On \u4e00\u6708 12, 2017, 3:32 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated \u4e00\u6708 12, 2017, 3:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Bad patch!

Reviews applied: [55271, 55381]

Failed command: python support/apply-reviews.py -n -r 55381

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: '55381.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: '55381.patch'

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16699/console

- Mesos ReviewBot


On Jan. 12, 2017, 3:32 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2017, 3:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 16, 2017, 5:58 a.m., Jay Guo wrote:
> > src/master/master.cpp, line 2478
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603888#file1603888line2478>
> >
> >     I think we don't necessarily have `framework` here if framework information hasn't been reconstructed from agent failover yet. See https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2563-L2571
> >     
> >     I feel that we need to introduce a cross-checking validation that checks "new" frameworkInfo against "old" one if available. And this may be done in `updateFrameworkInfo`?
> 
> Benjamin Bannier wrote:
>     Very good point. I agree that moving all these checks to `updateFrameworkInfo` makes most sense as there we have all needed information, however it currently returns just `void` where instead we probably need a `Try<Nothing>`. Let me try to improve `updateFrameworkInfo` so we can check there.

I moved the validation to `updateFrameworkInfo` after changing it so it can return an error (in previous patch).


- Benjamin


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


On Jan. 16, 2017, 12:28 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2017, 12:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Jay Guo <gu...@gmail.com>.


> On Jan. 16, 2017, 12:58 p.m., Jay Guo wrote:
> > src/master/master.cpp, lines 2491-2492
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603888#file1603888line2491>
> >
> >     Note that you could use `framework.capabilities` here since https://reviews.apache.org/r/55021/ is landed.
> 
> Benjamin Bannier wrote:
>     Done, also for the other call to `protobuf::frameworkHasCapabilities` added as part of this patch.
>     
>     We should probably also the remove the other uses of `protobuf::frameworkHasCapabilities`. Do you already have a patch for that?

Some of them are done in https://reviews.apache.org/r/55066/ and https://reviews.apache.org/r/55068/

feel free to post another patch if I miss something.


- Jay


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


On Jan. 16, 2017, 5:54 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b863ff6e93931c3d1ee056248084c7f44caf2fd9 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On Jan. 16, 2017, 5:58 a.m., Jay Guo wrote:
> > I wonder how we deal with default role `*` in multi-role scenario? I remember that we require `*` to be explicitly specified for a multi-role framework, is that still true?

This is not how I read the design doc, but this is only touched on in passing, e.g.,
> Because we cannot distinguish between an empty set of roles (new-style framework wanting no roles) and an unset role (old-style framework wanting the "*" role), we must introduce a framework capability (i.e. MULTI_ROLE). 

I interpret this to mean that a framework not wanting a particular role passes an empty `roles` set.


> On Jan. 16, 2017, 5:58 a.m., Jay Guo wrote:
> > src/master/master.cpp, lines 2491-2492
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603888#file1603888line2491>
> >
> >     Note that you could use `framework.capabilities` here since https://reviews.apache.org/r/55021/ is landed.

Done, also for the other call to `protobuf::frameworkHasCapabilities` added as part of this patch.

We should probably also the remove the other uses of `protobuf::frameworkHasCapabilities`. Do you already have a patch for that?


> On Jan. 16, 2017, 5:58 a.m., Jay Guo wrote:
> > src/master/master.cpp, line 2478
> > <https://reviews.apache.org/r/55271/diff/5/?file=1603888#file1603888line2478>
> >
> >     I think we don't necessarily have `framework` here if framework information hasn't been reconstructed from agent failover yet. See https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2563-L2571
> >     
> >     I feel that we need to introduce a cross-checking validation that checks "new" frameworkInfo against "old" one if available. And this may be done in `updateFrameworkInfo`?

Very good point. I agree that moving all these checks to `updateFrameworkInfo` makes most sense as there we have all needed information, however it currently returns just `void` where instead we probably need a `Try<Nothing>`. Let me try to improve `updateFrameworkInfo` so we can check there.


- Benjamin


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


On Jan. 12, 2017, 4:32 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2017, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Jay Guo <gu...@gmail.com>.

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



I wonder how we deal with default role `*` in multi-role scenario? I remember that we require `*` to be explicitly specified for a multi-role framework, is that still true?


src/master/master.cpp (line 2478)
<https://reviews.apache.org/r/55271/#comment232995>

    I think we don't necessarily have `framework` here if framework information hasn't been reconstructed from agent failover yet. See https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2563-L2571
    
    I feel that we need to introduce a cross-checking validation that checks "new" frameworkInfo against "old" one if available. And this may be done in `updateFrameworkInfo`?



src/master/master.cpp (lines 2491 - 2492)
<https://reviews.apache.org/r/55271/#comment232996>

    Note that you could use `framework.capabilities` here since https://reviews.apache.org/r/55021/ is landed.


- Jay Guo


On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2017, 11:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 12, 2017, 4:32 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Rebased.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 11, 2017, 11:37 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Rebased, added proper dep on r/55381.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 10, 2017, 2:49 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2017, 2:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 10, 2017, 3:49 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Fixed upgrade to multi-role.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated Jan. 10, 2017, 1:29 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
-------

Addressed gyliu's review comments.


Bugs: MESOS-6631
    https://issues.apache.org/jira/browse/MESOS-6631


Repository: mesos


Description
-------

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-----

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
-------

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 6, 2017, 7:07 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 11c34a048586d30c6ac67be8638ed8fa81cc3f1f 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>