> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote: > > src/tests/containerizer/isolator_tests.cpp, line 378 > > <https://reviews.apache.org/r/50271/diff/15/?file=1515280#file1515280line378> > > > > I suggest we take `Option<vector<Capability>>` here so that the caller > > can do: > > ``` > > TestConfig( > > {NET_RAW, SYS_ADMIN}, > > {...}, > > ... > > ) > > ```
I have changed this to now take `Option<set<Capability>>` since that's what `capabilities::convert` accepts. > On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote: > > src/tests/containerizer/isolator_tests.cpp, line 381 > > <https://reviews.apache.org/r/50271/diff/15/?file=1515280#file1515280line381> > > > > I would simply this to be a bool: if the task should finish normally or > > not. > > > > In the test, I would watch for terminal status update and see if it's > > TASK_FINISHED or not. With the test in its current form this cannot be a `bool` since a task might fail in different ways (failure to start, or return with a failure when run). Depending on the number of task status updates we expect we do `EXPECT` differently. In its current form we can be both explicit about when we expect a task to fail, and also avoid introducing an extra parameter storing the number of task status updates we expect. Can we leave it like is? > On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote: > > src/tests/containerizer/isolator_tests.cpp, lines 573-578 > > <https://reviews.apache.org/r/50271/diff/15/?file=1515280#file1515280line573> > > > > This can be > > ``` > > TestConfig({}, None(), false) > > ``` See above. > On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote: > > src/tests/containerizer/isolator_tests.cpp, lines 657-659 > > <https://reviews.apache.org/r/50271/diff/15/?file=1515280#file1515280line657> > > > > Should we fix those? In fact I already did (https://reviews.apache.org/r/51931/), but failed to remove the `FIXME`. > On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote: > > src/tests/containerizer/isolator_tests.cpp, line 380 > > <https://reviews.apache.org/r/50271/diff/15/?file=1515280#file1515280line380> > > > > I would remove this and leave a TODO. We currently have a test for the base case relying on this (the first one in `NoCapabilitiesConfigured`). Is your suggestion to remove that test case? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50271/#review150223 ----------------------------------------------------------- On Oct. 4, 2016, 3:08 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50271/ > ----------------------------------------------------------- > > (Updated Oct. 4, 2016, 3:08 p.m.) > > > Review request for mesos, Jay Guo and Jie Yu. > > > Bugs: MESOS-5275 > https://issues.apache.org/jira/browse/MESOS-5275 > > > Repository: mesos > > > Description > ------- > > This isolator evaluates agent allowed capabilities and passes net > capabilities on to `mesos-containerizer` which enforces the > capabilities. > > Capability information is passed via a new field in > `ContainerLaunchInfo`. > > > Diffs > ----- > > include/mesos/slave/containerizer.proto > 86d1859854b44f237ac2ca1ef74982b543c08d25 > src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 > src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b > src/slave/containerizer/mesos/containerizer.cpp > e6bd9f7a8284d220be157a3db2da094e6b1b6d33 > src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION > src/slave/containerizer/mesos/launch.hpp > a9b6ee2f0da2c1c84c6a0642adbce20a9f0218dc > src/slave/containerizer/mesos/launch.cpp > 7dd10d5030260fbfdf7396a7c05a52b7c3d983e8 > src/tests/containerizer/capabilities_isolator_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/50271/diff/ > > > Testing > ------- > > `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o > optimizations) > `make` (OS X, clang-4.0 w/o optimizations) > > > Thanks, > > Benjamin Bannier > >
