Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.

2015-12-18 Thread Greg Mann


> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, line 790
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 

Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.

2015-12-18 Thread Alexander Rukletsov

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



src/master/master.cpp (lines 3346 - 3350)


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.

2015-12-18 Thread Greg Mann

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

2015-12-18 Thread Greg Mann


> On Dec. 18, 2015, 9:58 a.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3346-3350
> > 
> >
> > 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/#review58
---


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.

2015-12-18 Thread Greg Mann

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

2015-12-18 Thread Greg Mann

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

2015-12-18 Thread Greg Mann

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

2015-12-18 Thread Alexander Rukletsov


> On Dec. 16, 2015, 2:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 3348
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

2015-12-18 Thread Alexander Rukletsov

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



src/tests/persistent_volume_tests.cpp (lines 887 - 890)


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)


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)


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)


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)


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.

2015-12-18 Thread Jie Yu

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


Thanks!



src/tests/persistent_volume_tests.cpp (line 751)


If you pause the clock, looks like this is not needed?



src/tests/persistent_volume_tests.cpp (line 906)


Ditto. Using default value sounds ok to me since clock is paused.



src/tests/persistent_volume_tests.cpp (lines 969 - 970)


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)


Can you move this up and right below `AWAIT_READY(checkpointResources1)`? 
Here and every other occurances.



src/tests/persistent_volume_tests.cpp (line 1007)


Ditto.



src/tests/persistent_volume_tests.cpp (lines 1016 - 1018)


Ditto.



src/tests/persistent_volume_tests.cpp (line 1069)


Why this given the clock is paused.



src/tests/persistent_volume_tests.cpp (line 1138)


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

2015-12-18 Thread Greg Mann

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

2015-12-18 Thread Greg Mann


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

2015-12-18 Thread Greg Mann


> On Dec. 18, 2015, 10:48 a.m., Alexander Rukletsov wrote:
> > src/tests/persistent_volume_tests.cpp, lines 888-891
> > 
> >
> > 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/#review63
---


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.

2015-12-16 Thread Alexander Rukletsov

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



src/master/master.cpp (lines 3141 - 3142)


Blank line



src/master/master.cpp (lines 3153 - 3154)


Blank line



src/master/master.cpp (line 3342)


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)


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)


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)


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)


You don't need the `process::` prefix. Here and everywhere.



src/tests/persistent_volume_tests.cpp (line 759)


Mind adding a comment why you statically reserve disk resources?



src/tests/persistent_volume_tests.cpp (line 790)


Do you think it makes sense to extract "role1" into a constant?



src/tests/persistent_volume_tests.cpp (lines 821 - 822)


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)


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)


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.

2015-12-16 Thread Alexander Rukletsov

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



Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.

2015-12-16 Thread Greg Mann


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

2015-12-08 Thread Greg Mann

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

2015-11-13 Thread Greg Mann

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

(Updated Nov. 14, 2015, 12:25 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 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/persistent_volume_tests.cpp 
8b791ac0861171b0c655307e965165d9ad7ba966 

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.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann



Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.

2015-11-12 Thread Greg Mann

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

(Updated Nov. 13, 2015, 1:21 a.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Summary (updated)
-

[5/7] Added framework authorization for persistent volumes.


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 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/persistent_volume_tests.cpp 
8b791ac0861171b0c655307e965165d9ad7ba966 

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


Testing
---

This is the fifth in a chain of 6 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.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann



Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.

2015-11-12 Thread Greg Mann

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

(Updated Nov. 13, 2015, 1:21 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 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/persistent_volume_tests.cpp 
8b791ac0861171b0c655307e965165d9ad7ba966 

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.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann