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 2015/12/08 20:03:14 UTC
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 7:03 p.m.)
Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
Changes
-------
Added manual clock control to tests.
Bugs: MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3065
Repository: mesos
Description
-------
Added framework authorization for persistent volumes.
Diffs (updated)
-----
src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
Diff: https://reviews.apache.org/r/40255/diff/
Testing (updated)
-------
This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, line 790
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line790>
> >
> > Do you think it makes sense to extract "role1" into a constant?
Since this role name appears throughout the file, I think I'd rather follow these up with a patch that makes this change for all of the persistent volume tests. What do you think?
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 3342
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156233#file1156233line3342>
> >
> > I see that you follow the pattern here, but why does the master commit suicide if the future fails? My understanding is that this does not violate any internal invariant and should lead to a retry. Am I missing something? Do we need a comment explaining the reason?
Thanks for calling this out - I agree that it's an error and should be removed from the RESERVE/UNRESERVE code as well. Check out what I've put in place of this and see what you think. I can submit a patch for RESERVE/UNRESERVE also.
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 3348
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156233#file1156233line3348>
> >
> > You are following the pattern here, but are we sure that the framework has the principal? I also do not see any tests with frameworks without principals (nor in "reservation_tests.cpp"). It looks like an unsuccessful authorization for a framework without a principal can kill the master.
I added tests without a principal, but this code shouldn't lead to a crash of the master. `principal` is an optional field in `FrameworkInfo`, which means that if it isn't supplied, it will be initialized with the default value: an empty string. So if the framework has no principal, this will result in the logging output: "Authorization of principal '' to create persistent volumes failed", which seems OK to me. I'm going to drop this for now, but feel free to re-open if I'm missing something or if you disagree for another reason.
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3348-3349
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156233#file1156233line3348>
> >
> > I believe we need an extra blank line in this case. Also in other places above and below : )
I changed this in the current patch, and I'll follow up with another for RESERVE/UNRESERVE.
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, lines 717-719
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line717>
> >
> > Could you please add a test with a framework without a principal?
> >
> > In the same vein, do you think it makes sense to create a ticket for the same case for dynamic reservatons (even though we require the principal for now)?
Excellent idea, I've added two tests to this patch for cases with no principal and created a ticket for RESERVE/UNRESERVE: https://issues.apache.org/jira/browse/MESOS-4195
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, lines 968-970
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line968>
> >
> > Do you think it makes sense to merge this test with the first one by creating a second framework with a different principal and allowing only that framework to destroy the volume?
Good idea! Done.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review110658
-----------------------------------------------------------
On Dec. 18, 2015, 9:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 9:28 a.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 3348
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156233#file1156233line3348>
> >
> > You are following the pattern here, but are we sure that the framework has the principal? I also do not see any tests with frameworks without principals (nor in "reservation_tests.cpp"). It looks like an unsuccessful authorization for a framework without a principal can kill the master.
>
> Greg Mann wrote:
> I added tests without a principal, but this code shouldn't lead to a crash of the master. `principal` is an optional field in `FrameworkInfo`, which means that if it isn't supplied, it will be initialized with the default value: an empty string. So if the framework has no principal, this will result in the logging output: "Authorization of principal '' to create persistent volumes failed", which seems OK to me. I'm going to drop this for now, but feel free to re-open if I'm missing something or if you disagree for another reason.
You are right, my bad.
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, lines 717-719
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line717>
> >
> > Could you please add a test with a framework without a principal?
> >
> > In the same vein, do you think it makes sense to create a ticket for the same case for dynamic reservatons (even though we require the principal for now)?
>
> Greg Mann wrote:
> Excellent idea, I've added two tests to this patch for cases with no principal and created a ticket for RESERVE/UNRESERVE: https://issues.apache.org/jira/browse/MESOS-4195
Thanks! I've noticed we usually do not test cases like "authn is off, authz is on, framework has a principal", "authn is off, authz if off, framework has no principal", though, I would say, are real-world scenarios (for test clusters only I hope : ) ).
> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, line 790
> > <https://reviews.apache.org/r/40255/diff/3/?file=1156234#file1156234line790>
> >
> > Do you think it makes sense to extract "role1" into a constant?
>
> Greg Mann wrote:
> Since this role name appears throughout the file, I think I'd rather follow these up with a patch that makes this change for all of the persistent volume tests. What do you think?
That's fine.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review110658
-----------------------------------------------------------
On Dec. 18, 2015, 9:37 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 9:37 a.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review110658
-----------------------------------------------------------
src/master/master.cpp (lines 3141 - 3142)
<https://reviews.apache.org/r/40255/#comment170693>
Blank line
src/master/master.cpp (lines 3153 - 3154)
<https://reviews.apache.org/r/40255/#comment170694>
Blank line
src/master/master.cpp (line 3342)
<https://reviews.apache.org/r/40255/#comment170696>
I see that you follow the pattern here, but why does the master commit suicide if the future fails? My understanding is that this does not violate any internal invariant and should lead to a retry. Am I missing something? Do we need a comment explaining the reason?
src/master/master.cpp (line 3348)
<https://reviews.apache.org/r/40255/#comment170697>
You are following the pattern here, but are we sure that the framework has the principal? I also do not see any tests with frameworks without principals (nor in "reservation_tests.cpp"). It looks like an unsuccessful authorization for a framework without a principal can kill the master.
src/master/master.cpp (lines 3348 - 3349)
<https://reviews.apache.org/r/40255/#comment170698>
I believe we need an extra blank line in this case. Also in other places above and below : )
src/tests/persistent_volume_tests.cpp (lines 717 - 719)
<https://reviews.apache.org/r/40255/#comment170704>
Could you please add a test with a framework without a principal?
In the same vein, do you think it makes sense to create a ticket for the same case for dynamic reservatons (even though we require the principal for now)?
src/tests/persistent_volume_tests.cpp (line 723)
<https://reviews.apache.org/r/40255/#comment170699>
You don't need the `process::` prefix. Here and everywhere.
src/tests/persistent_volume_tests.cpp (line 759)
<https://reviews.apache.org/r/40255/#comment170700>
Mind adding a comment why you statically reserve disk resources?
src/tests/persistent_volume_tests.cpp (line 790)
<https://reviews.apache.org/r/40255/#comment170701>
Do you think it makes sense to extract "role1" into a constant?
src/tests/persistent_volume_tests.cpp (lines 821 - 822)
<https://reviews.apache.org/r/40255/#comment170702>
Let's either insert a blank line or refactor the comment so that it refers to both `EXPECT_*`s. Same below.
src/tests/persistent_volume_tests.cpp (lines 869 - 871)
<https://reviews.apache.org/r/40255/#comment170703>
Only for a framework with a principal. Can we add one more test with a framework *without* a principal? I believe now it will lead to a master crash : )
src/tests/persistent_volume_tests.cpp (lines 968 - 970)
<https://reviews.apache.org/r/40255/#comment170705>
Do you think it makes sense to merge this test with the first one by creating a second framework with a different principal and allowing only that framework to destroy the volume?
- Alexander Rukletsov
On Dec. 8, 2015, 7:03 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 7:03 p.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 18, 2015, 7:59 p.m., Jie Yu wrote:
> >
Thanks for the reviews, Jie!
> On Dec. 18, 2015, 7:59 p.m., Jie Yu wrote:
> > src/tests/persistent_volume_tests.cpp, line 751
> > <https://reviews.apache.org/r/40255/diff/7/?file=1170740#file1170740line751>
> >
> > If you pause the clock, looks like this is not needed?
Yes, correct, good call. I posted another patch that defines a `DEFAULT_ALLOCATION_INTERVAL` and exposes it in `master/constants.hpp` so that we can make use of it: https://reviews.apache.org/r/41573/ This will be useful as we migrate more tests to control the clock explicitly.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review111233
-----------------------------------------------------------
On Dec. 18, 2015, 11:12 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 11:12 p.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review111233
-----------------------------------------------------------
Ship it!
src/master/master.cpp (line 3425)
<https://reviews.apache.org/r/40255/#comment171326>
Thanks!
src/tests/persistent_volume_tests.cpp (line 751)
<https://reviews.apache.org/r/40255/#comment171330>
If you pause the clock, looks like this is not needed?
src/tests/persistent_volume_tests.cpp (line 906)
<https://reviews.apache.org/r/40255/#comment171335>
Ditto. Using default value sounds ok to me since clock is paused.
src/tests/persistent_volume_tests.cpp (lines 969 - 970)
<https://reviews.apache.org/r/40255/#comment171338>
I would move this up right below 'acceptOffers' to have better flow (first volume is created, and then we receive another offer). Here, and every other occurances.
src/tests/persistent_volume_tests.cpp (lines 980 - 981)
<https://reviews.apache.org/r/40255/#comment171341>
Can you move this up and right below `AWAIT_READY(checkpointResources1)`? Here and every other occurances.
src/tests/persistent_volume_tests.cpp (line 1007)
<https://reviews.apache.org/r/40255/#comment171342>
Ditto.
src/tests/persistent_volume_tests.cpp (lines 1016 - 1018)
<https://reviews.apache.org/r/40255/#comment171343>
Ditto.
src/tests/persistent_volume_tests.cpp (line 1069)
<https://reviews.apache.org/r/40255/#comment171345>
Why this given the clock is paused.
src/tests/persistent_volume_tests.cpp (line 1138)
<https://reviews.apache.org/r/40255/#comment171346>
+1 on using suppressOffer and reviveOffer
- Jie Yu
On Dec. 18, 2015, 10:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 10:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 11:12 p.m.)
Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
Changes
-------
Addressed comments.
Bugs: MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3065
Repository: mesos
Description
-------
Added framework authorization for persistent volumes.
Diffs (updated)
-----
src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
Diff: https://reviews.apache.org/r/40255/diff/
Testing
-------
This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 18, 2015, 10:48 a.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, lines 888-891
> > <https://reviews.apache.org/r/40255/diff/3-6/?file=1156234#file1156234line888>
> >
> > It looks like this rule is not used in this test. Could you please explain why you have it here?
Ah whoops, yes this is incorrect. I changed it to the appropriate ACL.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review111163
-----------------------------------------------------------
On Dec. 18, 2015, 11:12 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 11:12 p.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review111163
-----------------------------------------------------------
src/tests/persistent_volume_tests.cpp (lines 887 - 890)
<https://reviews.apache.org/r/40255/#comment171190>
It looks like this rule is not used in this test. Could you please explain why you have it here?
src/tests/persistent_volume_tests.cpp (line 908)
<https://reviews.apache.org/r/40255/#comment171188>
I would merge this comment with the one on L903, this way you can avoid inserting a missing blank line between L907 and L908 : ).
Same below.
src/tests/persistent_volume_tests.cpp (lines 978 - 981)
<https://reviews.apache.org/r/40255/#comment171189>
I think you decided to rewrite this block a bit.
Same for other EXPECT blobs with comments.
src/tests/persistent_volume_tests.cpp (lines 1134 - 1135)
<https://reviews.apache.org/r/40255/#comment171192>
Instead of "magic constants", which may lead to flaky tests, does it make sense to `suppressOffers` and then `reviveOffers`?
Same for the next test.
src/tests/persistent_volume_tests.cpp (lines 1185 - 1187)
<https://reviews.apache.org/r/40255/#comment171193>
Why do you think framework1 will be offered resources?
- Alexander Rukletsov
On Dec. 18, 2015, 10:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 10:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 10:10 a.m.)
Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
Changes
-------
Addressed comment.
Bugs: MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3065
Repository: mesos
Description
-------
Added framework authorization for persistent volumes.
Diffs (updated)
-----
src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
Diff: https://reviews.apache.org/r/40255/diff/
Testing
-------
This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 18, 2015, 9:58 a.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3346-3350
> > <https://reviews.apache.org/r/40255/diff/6/?file=1170703#file1170703line3346>
> >
> > How about a TODO, that we may want to retry instead of giving up straight away?
Yea, sounds good, TODOs added!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review111158
-----------------------------------------------------------
On Dec. 18, 2015, 10:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 10:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review111158
-----------------------------------------------------------
src/master/master.cpp (lines 3346 - 3350)
<https://reviews.apache.org/r/40255/#comment171183>
How about a TODO, that we may want to retry instead of giving up straight away?
- Alexander Rukletsov
On Dec. 18, 2015, 9:37 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 9:37 a.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 9:37 a.m.)
Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
Changes
-------
Rebase.
Bugs: MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3065
Repository: mesos
Description
-------
Added framework authorization for persistent volumes.
Diffs (updated)
-----
src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
Diff: https://reviews.apache.org/r/40255/diff/
Testing
-------
This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 9:33 a.m.)
Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
Changes
-------
`process::Clock` -> `Clock`
Bugs: MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3065
Repository: mesos
Description
-------
Added framework authorization for persistent volumes.
Diffs (updated)
-----
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
Diff: https://reviews.apache.org/r/40255/diff/
Testing
-------
This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 9:28 a.m.)
Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
Changes
-------
Addressed comments.
Bugs: MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3065
Repository: mesos
Description
-------
Added framework authorization for persistent volumes.
Diffs (updated)
-----
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
Diff: https://reviews.apache.org/r/40255/diff/
Testing
-------
This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
Thanks,
Greg Mann
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 16, 2015, 3:24 p.m., Alexander Rukletsov wrote:
> > And what about master endpoints (authz for operators)? I can't find that code in the chain of reviews. Will it be done separately?
Yep, there is a separate ticket for that: https://issues.apache.org/jira/browse/MESOS-3903
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review110664
-----------------------------------------------------------
On Dec. 8, 2015, 7:03 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 7:03 p.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 40255: [5/7] Added framework authorization for
persistent volumes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40255/#review110664
-----------------------------------------------------------
And what about master endpoints (authz for operators)? I can't find that code in the chain of reviews. Will it be done separately?
- Alexander Rukletsov
On Dec. 8, 2015, 7:03 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 7:03 p.m.)
>
>
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
>
>
> Bugs: MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added framework authorization for persistent volumes.
>
>
> Diffs
> -----
>
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/tests/persistent_volume_tests.cpp 01b3c13751a5558d5f06edb8f650c8644dc54486
>
> Diff: https://reviews.apache.org/r/40255/diff/
>
>
> Testing
> -------
>
> This is the fifth in a chain of 7 patches. New tests were added to `persistent_volume_tests.cpp` in order to test a framework attempting both successful and failed authorizations for `CREATE` and `DESTROY` offer operations. `make check` was used to test after all patches were applied.
>
>
> Thanks,
>
> Greg Mann
>
>