Re: Review Request 70765: Renamed struct `ProfileRecord` to `ProfileData` for consistency.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70766, 70765]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 31, 2019, 3:12 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70765/
> ---
> 
> (Updated May 31, 2019, 3:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed struct `ProfileRecord` to `ProfileData` for consistency.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 215f7f92b5c2a0e60555134ce9887e8a187e3b1d 
> 
> 
> Diff: https://reviews.apache.org/r/70765/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70757: Added a privs isolator.

[James Peach]

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



You should add a test in this review. I'd probably do something reasonable 
simple, like launch a task and verify that the `NoNewPrivs` fiels in 
`/proc/$PID/status` is what you expect it to be. Take a look at 
`src/tests/containerizer/linux_filesystem_isolator_tests.cpp` for an example of 
how to construct the test by driving the containerizer directly.

We also need to add feature and upgrade documentation, but if you want to do 
that in a later review, that's fine with me.


include/mesos/slave/containerizer.proto
Lines 309 (patched)


The field number needs to be updated, since intermediate changes allocated 
21.



src/slave/containerizer/mesos/isolators/linux/privs.hpp
Lines 30 (patched)


I did originally advise you to call this isolator something more open 
ended, because I had it in mind that we could extend this to toggle secbits as 
well.

I've reconsidered and I think that I gave you bad advice. Let's check in 
with the other containerization folks, but I'm inclined to call this 
`linux/nnp` and leave it at that.

Sorry for the back & forth.



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 17 (patched)


Do you need this (once you drop the geteuid check)?



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 19 (patched)


Do you need this?



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 24 (patched)


I think you can remove all or most of these headers too.



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 44 (patched)


You don't need roo to set the NNP flag, so we can drop this.



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 49 (patched)


We don't need this check. The NNP flag will work with any launcher, as long 
as the host OS is Linux.



src/slave/containerizer/mesos/launch.cpp
Lines 1101 (patched)


A more succinct phrasing would be:
```
if (prctl(...) == -1) {
  cerr << ErrnoError("Failed to set the NO_NEW_PRIVS flag"), << endl;
  exitWithStatus(EXIT_FAILURE);
}
```


- James Peach


On May 31, 2019, 4:02 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated May 31, 2019, 4:02 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> b2e35cbf01caea6c1e4f45c7b7d833bc7f065099 
>   src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 043244841a73fa3f5f7119bc38f6d3a04be8990b 
>   src/slave/containerizer/mesos/isolators/linux/privs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/privs.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> 0c482f46a97063133edfe29ae3c6a2721d29f6c6 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Review Request 70765: Renamed struct `ProfileRecord` to `ProfileData` for consistency.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Repository: mesos


Description
---

Renamed struct `ProfileRecord` to `ProfileData` for consistency.


Diffs
-

  src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3 
  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
215f7f92b5c2a0e60555134ce9887e8a187e3b1d 


Diff: https://reviews.apache.org/r/70765/diff/1/


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Review Request 70766: Fixed chaining futures infinitely in `UriDiskProfileAdaptor`.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Bugs: MESOS-9803
https://issues.apache.org/jira/browse/MESOS-9803


Repository: mesos


Description
---

Previously it is possible to have an infinite chain of futures when
`UriDiskProfileAdaptor::watch` is called: if the set of profiles remains
fixed for every poll, each poll would satisfy a promise that triggers
an asynchronous recursive call to `UriDiskProfileAdaptor::watch` again.

This patch fixes the problem by removing the asynchronous recursion.
Instead, we maintain a separated promise for each watcher that is never
associated to another promise. After each poll, we check if the current
set of profiles differs from the known set for a watcher, and satisfy
its own promise if so.


Diffs
-

  src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3 
  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
215f7f92b5c2a0e60555134ce9887e8a187e3b1d 


Diff: https://reviews.apache.org/r/70766/diff/1/


Testing
---

make check

Manually tested with the unit test in r/70764. The unit test will fail at the 
5th poll without the fix and will pass with the fix.


Thanks,

Chun-Hung Hsiao