You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2016/02/17 21:44:25 UTC

Review Request 43639: Allowed dynamic reservation without a principal.

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

Review request for mesos, Michael Park and Neil Conway.


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


Repository: mesos


Description
-------

Allowed dynamic reservation without a principal.

The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.

Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. This isn't necessary, and is tracked in MESOS-4696.


Diffs
-----

  src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
  src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
  src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
  src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 

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


Testing
-------

`make check`

Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`

Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.


Thanks,

Greg Mann


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Greg Mann <gr...@mesosphere.io>.

> On Feb. 18, 2016, 6:57 a.m., Guangya Liu wrote:
> > src/master/validation.cpp, lines 692-711
> > <https://reviews.apache.org/r/43639/diff/1/?file=1253091#file1253091line692>
> >
> >     Can you please add some test cases to cover those scenarios?
> >     
> >     1) authentication is enabled and reservation does not have principal.
> >     2) authentication is enabled and reservation pricipal does not same as authentication principal.
> >     3) authentication is disabled but reservation has principal.
> 
> Guangya Liu wrote:
>     Seems 2) was covered by `ReservationEndpointsTest, NonMatchingPrincipal`, but 1) and 3) was not covered for now.

Good call, I added a test case to the `ReservationTest` tests for #3.

Due to the way authentication works, I'm not sure that test case #1 is necessary. When authentication is enabled, HTTP endpoints require a principal and frameworks must supply a principal to register. The best we could do to test #1 is to have a request with an authenticated principal but no principal in `ReservationInfo`, which is really just testing that a request with non-matching principals will fail. The non-matching principal case is covered by `ReservationEndpointsTest.NonMatchingPrincipal`. What do you think?


- Greg


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


On Feb. 22, 2016, 6:39 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2016, 6:39 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> A new test case was added to `ReservationTest.NoAuthentication`.
> 
> `make check` was used to test on OSX, both with and without SSL enabled.
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

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

> On 二月 18, 2016, 6:57 a.m., Guangya Liu wrote:
> > src/master/validation.cpp, lines 692-711
> > <https://reviews.apache.org/r/43639/diff/1/?file=1253091#file1253091line692>
> >
> >     Can you please add some test cases to cover those scenarios?
> >     
> >     1) authentication is enabled and reservation does not have principal.
> >     2) authentication is enabled and reservation pricipal does not same as authentication principal.
> >     3) authentication is disabled but reservation has principal.
> 
> Guangya Liu wrote:
>     Seems 2) was covered by `ReservationEndpointsTest, NonMatchingPrincipal`, but 1) and 3) was not covered for now.
> 
> Greg Mann wrote:
>     Good call, I added a test case to the `ReservationTest` tests for #3.
>     
>     Due to the way authentication works, I'm not sure that test case #1 is necessary. When authentication is enabled, HTTP endpoints require a principal and frameworks must supply a principal to register. The best we could do to test #1 is to have a request with an authenticated principal but no principal in `ReservationInfo`, which is really just testing that a request with non-matching principals will fail. The non-matching principal case is covered by `ReservationEndpointsTest.NonMatchingPrincipal`. What do you think?

fair enough, thanks Greg.


- Guangya


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


On 二月 23, 2016, 12:16 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated 二月 23, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> A new test case was added to `ReservationTest.NoAuthentication`.
> 
> `make check` was used to test on OSX, both with and without SSL enabled.
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

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

> On 二月 18, 2016, 6:57 a.m., Guangya Liu wrote:
> > src/master/validation.cpp, lines 692-711
> > <https://reviews.apache.org/r/43639/diff/1/?file=1253091#file1253091line692>
> >
> >     Can you please add some test cases to cover those scenarios?
> >     
> >     1) authentication is enabled and reservation does not have principal.
> >     2) authentication is enabled and reservation pricipal does not same as authentication principal.
> >     3) authentication is disabled but reservation has principal.

Seems 2) was covered by `ReservationEndpointsTest, NonMatchingPrincipal`, but 1) and 3) was not covered for now.


- Guangya


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


On 二月 17, 2016, 8:45 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated 二月 17, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. This isn't necessary, and removing that requirement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/#review119589
-----------------------------------------------------------




src/master/validation.cpp (lines 675 - 694)
<https://reviews.apache.org/r/43639/#comment180950>

    Can you please add some test cases to cover those scenarios?
    
    1) authentication is enabled and reservation does not have principal.
    2) authentication is enabled and reservation pricipal does not same as authentication principal.
    3) authentication is disabled but reservation has principal.



src/tests/master_validation_tests.cpp (line 385)
<https://reviews.apache.org/r/43639/#comment180946>

    s/principal/`principal`


- Guangya Liu


On 二月 17, 2016, 8:45 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated 二月 17, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. This isn't necessary, and removing that requirement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/#review120207
-----------------------------------------------------------




src/tests/reservation_endpoints_tests.cpp (line 1269)
<https://reviews.apache.org/r/43639/#comment181633>

    s/No/Without/ ?



src/tests/reservation_tests.cpp (lines 1860 - 1866)
<https://reviews.apache.org/r/43639/#comment181641>

    can you split this into 2 tests, one with principal and another without principal?



src/tests/reservation_tests.cpp (line 1876)
<https://reviews.apache.org/r/43639/#comment181642>

    this doesnt have to change?



src/tests/reservation_tests.cpp (line 1979)
<https://reviews.apache.org/r/43639/#comment181643>

    offer with reserved resources?



src/tests/reservation_tests.cpp (line 1997)
<https://reviews.apache.org/r/43639/#comment181644>

    offer without reserved resources.


- Vinod Kone


On Feb. 22, 2016, 6:39 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2016, 6:39 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> A new test case was added to `ReservationTest.NoAuthentication`.
> 
> `make check` was used to test on OSX, both with and without SSL enabled.
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/#review120209
-----------------------------------------------------------


Fix it, then Ship it!





src/tests/reservation_tests.cpp (line 1851)
<https://reviews.apache.org/r/43639/#comment181635>

    This should probably be split into two test cases, since AFAICS the first and second halves of the test don't share any state.



src/tests/reservation_tests.cpp (line 1949)
<https://reviews.apache.org/r/43639/#comment181634>

    Not yours, but I would probably opt for `using Process::Clock;` at the top of the file.


- Neil Conway


On Feb. 22, 2016, 6:39 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2016, 6:39 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> A new test case was added to `ReservationTest.NoAuthentication`.
> 
> `make check` was used to test on OSX, both with and without SSL enabled.
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/#review120233
-----------------------------------------------------------


Ship it!




Ship It!

- Michael Park


On Feb. 23, 2016, 12:16 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> A new test case was added to `ReservationTest.NoAuthentication`.
> 
> `make check` was used to test on OSX, both with and without SSL enabled.
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/#review120238
-----------------------------------------------------------


Fix it, then Ship it!





src/tests/reservation_tests.cpp (line 2000)
<https://reviews.apache.org/r/43639/#comment181660>

    one period


- Guangya Liu


On 二月 23, 2016, 12:16 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> -----------------------------------------------------------
> 
> (Updated 二月 23, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-3940
>     https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed dynamic reservation without a principal.
> 
> The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.
> 
> Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43639/diff/
> 
> 
> Testing
> -------
> 
> A new test case was added to `ReservationTest.NoAuthentication`.
> 
> `make check` was used to test on OSX, both with and without SSL enabled.
> 
> Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`
> 
> Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/
-----------------------------------------------------------

(Updated Feb. 23, 2016, 12:16 a.m.)


Review request for mesos, Michael Park and Neil Conway.


Changes
-------

Addressed comments.


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


Repository: mesos


Description
-------

Allowed dynamic reservation without a principal.

The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.

Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.


Diffs (updated)
-----

  src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
  src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
  src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
  src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 

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


Testing
-------

A new test case was added to `ReservationTest.NoAuthentication`.

`make check` was used to test on OSX, both with and without SSL enabled.

Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`

Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.


Thanks,

Greg Mann


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/
-----------------------------------------------------------

(Updated Feb. 22, 2016, 6:39 p.m.)


Review request for mesos, Michael Park and Neil Conway.


Changes
-------

Added a new test case.


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


Repository: mesos


Description (updated)
-------

Allowed dynamic reservation without a principal.

The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.

Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. It may be desirable to remove this requirement; this improvement is tracked in MESOS-4696.


Diffs (updated)
-----

  src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
  src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
  src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
  src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 

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


Testing (updated)
-------

A new test case was added to `ReservationTest.NoAuthentication`.

`make check` was used to test on OSX, both with and without SSL enabled.

Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`

Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.


Thanks,

Greg Mann


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/
-----------------------------------------------------------

(Updated Feb. 20, 2016, 1:53 a.m.)


Review request for mesos, Michael Park and Neil Conway.


Changes
-------

Added backticks.


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


Repository: mesos


Description
-------

Allowed dynamic reservation without a principal.

The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.

Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. This isn't necessary, and removing that requirement is tracked in MESOS-4696.


Diffs (updated)
-----

  src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
  src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
  src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
  src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 

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


Testing
-------

`make check`

Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`

Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.


Thanks,

Greg Mann


Re: Review Request 43639: Allowed dynamic reservation without a principal.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/
-----------------------------------------------------------

(Updated Feb. 17, 2016, 8:45 p.m.)


Review request for mesos, Michael Park and Neil Conway.


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


Repository: mesos


Description (updated)
-------

Allowed dynamic reservation without a principal.

The `ReservationInfo.principal` field has been migrated to `optional`, which means we can now allow dynamic reservation and unreservation without a principal. This allows the use of the `/reserve` and `/unreserve` HTTP endpoints when HTTP authentication is disabled.

Note that we still require that frameworks/operators set the `ReservationInfo.principal` field to match their own principal, if present. This isn't necessary, and removing that requirement is tracked in MESOS-4696.


Diffs
-----

  src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
  src/tests/master_validation_tests.cpp 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
  src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 
  src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 

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


Testing
-------

`make check`

Also manually reserved/unreserved resources using curl, with a command like this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d resources='[ { "name": "cpus",  "type": "SCALAR", "scalar": { "value": 3 }, "role": "ads", "reservation": { } } ]'  -X POST http://127.0.0.1:5050/master/reserve`

Inspecting `/master/state` before & after these operations confirmed that the reserve/unreserve operations were successful.


Thanks,

Greg Mann