mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 50271: Created an isolator for Linux capabilities.
Date Tue, 04 Oct 2016 13:08:50 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message