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