Re: Review Request 69723: Enabled operation feedback on agent default resources.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69723 was successfully built and tested.

Reviews applied: `['69786', '69723']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2802/mesos-review-69723

- Mesos Reviewbot Windows


On Jan. 18, 2019, 9:45 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69723/
> ---
> 
> (Updated Jan. 18, 2019, 9:45 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-9472
> https://issues.apache.org/jira/browse/MESOS-9472
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up to now, the master would validate that any operation
> requesting operation feedback would only act on resources
> belonging to a resource provider.
> 
> This patch removes this restriction and enables frameworks
> to request operation feedback on an agent's default
> resources.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2339207149a85578ea47cf66f28392182f9075f2 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/slave/slave.cpp 10cbc190acc7eea5734efa0066541168545a66b6 
>   src/tests/api_tests.cpp 79a2d2966ff6533629aeeefaaaf19e849b76e0e6 
>   src/tests/master_tests.cpp 1f8da63aa313c4679c0d7a3934802af1474e8f28 
>   src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 
> 
> 
> Diff: https://reviews.apache.org/r/69723/diff/3/
> 
> 
> Testing
> ---
> 
> ./src/mesos-tests
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 69735: Fixed maintenance causes machines not in schedule rescinding offers.

[fei long]

> On Jan. 14, 2019, 6:52 p.m., Joseph Wu wrote:
> > Here's a test I wrote for this issue: https://reviews.apache.org/r/65366/
> > The patch is a bit old, but could be re-used if necessary.
> 
> fei long wrote:
> Since your patch has not been merged, I copied your code and passed the 
> test by running "./bin/mesos-tests.sh --verbose 
> --gtest_filter="*RescindOffers*" --gtest_break_on_failure --gtest_repeat=100".

How could I re-trigger the CI?


- fei


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


On Jan. 14, 2019, 3:34 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69735/
> ---
> 
> (Updated Jan. 14, 2019, 3:34 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9394
> https://issues.apache.org/jira/browse/MESOS-9394
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed maintenance causes machines not in schedule rescinding offers.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
> 
> 
> Diff: https://reviews.apache.org/r/69735/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fei long
> 
>

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Benjamin Bannier]

> On Jan. 18, 2019, 4:22 a.m., Chun-Hung Hsiao wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3340 (patched)
> > 
> >
> > Could it be possible that the second `UpdateSlaveMessage` has been 
> > received before this `FUTURE_PROTOBUF` is set up?

Definitely, installing two expectations right away now instead.


- Benjamin


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


On Jan. 18, 2019, 12:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69781/
> ---
> 
> (Updated Jan. 18, 2019, 12:45 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we addressed one source of flakiness of this test in
> `2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
> flakiness (agents send more than the expected number of
> `UpdateSlaveMessage`s since they failed a timeout in registration with
> the master).
> 
> This patch ensures that the agent registers successfully before
> continuing with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 164e93a3749d4d668e12de31641aecece384 
> 
> 
> Diff: https://reviews.apache.org/r/69781/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Benjamin Bannier]

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

(Updated Jan. 18, 2019, 12:45 p.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Addressed issues raised by Chun.


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


Repository: mesos


Description
---

While we addressed one source of flakiness of this test in
`2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
flakiness (agents send more than the expected number of
`UpdateSlaveMessage`s since they failed a timeout in registration with
the master).

This patch ensures that the agent registers successfully before
continuing with the test.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
164e93a3749d4d668e12de31641aecece384 


Diff: https://reviews.apache.org/r/69781/diff/2/

Changes: https://reviews.apache.org/r/69781/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Benjamin Bannier]

> On Jan. 18, 2019, 4:42 a.m., Chun-Hung Hsiao wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3330 (patched)
> > 
> >
> > Is this really required?

This one conservative approach to deal with the possibility of them being 
different. Alternatively one could instead advance only by their maximum value. 
If at all we currently advance by both durations.

Dropping.


- Benjamin


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


On Jan. 18, 2019, 12:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69781/
> ---
> 
> (Updated Jan. 18, 2019, 12:45 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we addressed one source of flakiness of this test in
> `2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
> flakiness (agents send more than the expected number of
> `UpdateSlaveMessage`s since they failed a timeout in registration with
> the master).
> 
> This patch ensures that the agent registers successfully before
> continuing with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 164e93a3749d4d668e12de31641aecece384 
> 
> 
> Diff: https://reviews.apache.org/r/69781/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69723: Enabled operation feedback on agent default resources.

[Benno Evers]

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

(Updated Jan. 18, 2019, 1:45 p.m.)


Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Changes
---

Used new `AGENT_OPERATION_UPDATE` capability.


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


Repository: mesos


Description
---

Up to now, the master would validate that any operation
requesting operation feedback would only act on resources
belonging to a resource provider.

This patch removes this restriction and enables frameworks
to request operation feedback on an agent's default
resources.


Diffs (updated)
-

  src/master/master.cpp 2339207149a85578ea47cf66f28392182f9075f2 
  src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
  src/slave/slave.cpp 10cbc190acc7eea5734efa0066541168545a66b6 
  src/tests/api_tests.cpp 79a2d2966ff6533629aeeefaaaf19e849b76e0e6 
  src/tests/master_tests.cpp 1f8da63aa313c4679c0d7a3934802af1474e8f28 
  src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 


Diff: https://reviews.apache.org/r/69723/diff/3/

Changes: https://reviews.apache.org/r/69723/diff/2-3/


Testing
---

./src/mesos-tests


Thanks,

Benno Evers

Re: Review Request 69787: Added a proxy mode to the test CSI plugin.

[Benjamin Bannier]

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




src/examples/test_csi_plugin.cpp
Lines 115 (patched)


Could we add an example here?



src/examples/test_csi_plugin.cpp
Lines 925 (patched)


Could you add a comment here explaining its purpose?



src/examples/test_csi_plugin.cpp
Lines 930-931 (patched)


Just accept values here? Move construction of `unique_ptr` is cheap and 
possibly elided.



src/examples/test_csi_plugin.cpp
Lines 956-963 (patched)


Reordering these would make a little clearer what we have here, e.g., the 
following or sim.

 GenericServerContext serverContext;
 GenericServerAsyncReaderWriter serverReaderWriter;
 ClientContext clientContext;
 std::unique_ptr clientReader;

 State state;

 ByteBuffer request;

 ByteBuffer response;
 Status status;



src/examples/test_csi_plugin.cpp
Lines 970 (patched)


Could we give this a better name, e.g., `completionQueue`? That would be 
less gRPC'y, but more Mesos'y.



src/examples/test_csi_plugin.cpp
Lines 975 (patched)


Could you add a comment somewhere documenting the state machine? Probably 
here instead of in `Call`.



src/examples/test_csi_plugin.cpp
Lines 978 (patched)


Let's use a smart pointer to make clear that we pass ownership below, e.g.,

std::unique_ptr first(new Call);

// Pass ownership below.
... first.release() ...

(Btw, we pull `unique_ptr` into the current scope with a using decl, but 
still spell it out in full in most places).



src/examples/test_csi_plugin.cpp
Lines 990 (patched)


We should log something here as this could fail in general.



src/examples/test_csi_plugin.cpp
Lines 999 (patched)


Use a smart pointer.



src/examples/test_csi_plugin.cpp
Lines 1015-1016 (patched)


Spell out grpc types here?



src/examples/test_csi_plugin.cpp
Line 1020 (original), 1146 (patched)


I was wondering whether one would need to explicitly shut down the server 
and completion queue, or does that happen in their destructors? I.e., are we 
missing code here or do we just have an undocumented lifetime ordering?



src/examples/test_csi_plugin.cpp
Lines 1165 (patched)


Let's call out the type `unique_ptr` here.



src/examples/test_csi_plugin.cpp
Line 1025 (original), 1168 (patched)


nit: Not this patch, but this is the default and not required in C++.


- Benjamin Bannier


On Jan. 18, 2019, 6:46 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69787/
> ---
> 
> (Updated Jan. 18, 2019, 6:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9517
> https://issues.apache.org/jira/browse/MESOS-9517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `--proxy` flag is set, the test CSI plugin would forward all gRPC
> requests to the specified gRPC server URI, and return the response to
> the caller. This can be used to forward all CSI calls to a mock CSI
> plugin object in the test process.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp af183037b280bab65578a4c447196a9ccf261e32 
> 
> 
> Diff: https://reviews.apache.org/r/69787/diff/1/
> 
> 
> Testing
> ---
> 
> Manually tweaked the plugin to forward requests to itself and all tests pass.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69735: Fixed maintenance causes machines not in schedule rescinding offers.

[Benjamin Bannier]

> On Jan. 14, 2019, 7:52 p.m., Joseph Wu wrote:
> > Here's a test I wrote for this issue: https://reviews.apache.org/r/65366/
> > The patch is a bit old, but could be re-used if necessary.
> 
> fei long wrote:
> Since your patch has not been merged, I copied your code and passed the 
> test by running "./bin/mesos-tests.sh --verbose 
> --gtest_filter="*RescindOffers*" --gtest_break_on_failure --gtest_repeat=100".
> 
> fei long wrote:
> How could I re-trigger the CI?

This requires special Jenkins karma. I triggered a rebuild for you.


- Benjamin


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


On Jan. 14, 2019, 4:34 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69735/
> ---
> 
> (Updated Jan. 14, 2019, 4:34 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9394
> https://issues.apache.org/jira/browse/MESOS-9394
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed maintenance causes machines not in schedule rescinding offers.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
> 
> 
> Diff: https://reviews.apache.org/r/69735/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fei long
> 
>

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69781 was successfully built and tested.

Reviews applied: `['69781']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2801/mesos-review-69781

- Mesos Reviewbot Windows


On Jan. 18, 2019, 11:45 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69781/
> ---
> 
> (Updated Jan. 18, 2019, 11:45 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we addressed one source of flakiness of this test in
> `2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
> flakiness (agents send more than the expected number of
> `UpdateSlaveMessage`s since they failed a timeout in registration with
> the master).
> 
> This patch ensures that the agent registers successfully before
> continuing with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 164e93a3749d4d668e12de31641aecece384 
> 
> 
> Diff: https://reviews.apache.org/r/69781/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69158: Added an integration test for resource provider removal.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69158 was successfully built and tested.

Reviews applied: `['68147', '69158']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2800/mesos-review-69158

- Mesos Reviewbot Windows


On Jan. 18, 2019, 9:39 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Jan. 18, 2019, 9:39 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an integration test for resource provider removal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/8/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69082: Correctly propagated `close` failures in some instances.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69082]

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

- Mesos Reviewbot


On Jan. 16, 2019, 1:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69082/
> ---
> 
> (Updated Jan. 16, 2019, 1:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Alex Clemmer.
> 
> 
> Bugs: MESOS-9331
> https://issues.apache.org/jira/browse/MESOS-9331
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When e.g., writing to disk, errors from write might only manifest at
> ::close time. We update some instances to correctly propagate these
> errors instead of dropping them silently. We only propagate the
> ::close error if the write operation succeeded, otherwise we just
> propagate the error from the write operation.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/mktemp.hpp 
> 63b3d1a7720d07f877fa1d4eb7f32a548916637a 
>   3rdparty/stout/include/stout/os/write.hpp 
> f7538f94f5a953a7a90a05bc1d2f138b6c17f814 
>   3rdparty/stout/include/stout/protobuf.hpp 
> eb4adef56f1701e3c101284e05e4e6c66eef9180 
> 
> 
> Diff: https://reviews.apache.org/r/69082/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68018: Added `SeccompFilter` class.

[Andrei Budnik]

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

(Updated Jan. 18, 2019, 8 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


Changes
---

Turned off `no_new_privs` bit.


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


Repository: mesos


Description
---

`SeccompFilter` class is a wrapper for `libseccomp` API. Its main
purpose is to provide a translation of the `ContainerSeccompProfile`
message into calls of `libseccomp` API.


Diffs (updated)
-

  src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
  src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
  src/linux/seccomp/seccomp.hpp PRE-CREATION 
  src/linux/seccomp/seccomp.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/68018/diff/17/

Changes: https://reviews.apache.org/r/68018/diff/16-17/


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 69493: Documented the `linux/seccomp` isolator.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 67844.

Failed command: `python.exe .\support\apply-reviews.py -n -r 67844`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2803/mesos-review-69493

Relevant logs:

- 
[apply-review-67844.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2803/mesos-review-69493/logs/apply-review-67844.log):

```
error: missing binary patch data for '3rdparty/libseccomp-2.3.3.tar.gz'
error: binary patch does not apply to '3rdparty/libseccomp-2.3.3.tar.gz'
error: 3rdparty/libseccomp-2.3.3.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On Nov. 30, 2018, 4:33 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69493/
> ---
> 
> (Updated Nov. 30, 2018, 4:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9036
> https://issues.apache.org/jira/browse/MESOS-9036
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/isolators/linux-seccomp.md PRE-CREATION 
>   docs/mesos-containerizer.md d15e82583fa207ba78e9fc1e83da0cf1f469ec4e 
>   docs/upgrades.md e493aefb36ea7b9631af35179938d778dc47442a 
> 
> 
> Diff: https://reviews.apache.org/r/69493/diff/6/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69082: Correctly propagated `close` failures in some instances.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/os/posix/mktemp.hpp
Line 43 (original), 43 (patched)


s/path/template/ and s/string/path/ would be clearer?



3rdparty/stout/include/stout/os/posix/mktemp.hpp
Line 55 (original), 54 (patched)


This statement reads rather confusingly, I initially thought we were trying 
to return the error as a string

One reason is that probably s/path/template/ and s/string/path/ would be 
clearer

But also, per the comment above, returning the error directly here would be 
simpler?



3rdparty/stout/include/stout/os/write.hpp
Line 143 (original), 140-146 (patched)


ditto here



3rdparty/stout/include/stout/protobuf.hpp
Lines 163-164 (patched)


Assigning this to `write` makes this seem a bit confusing to read, 
logically we're trying to do:

```
if write was ok but close failed:
  return close error
  
return write
```

so perhaps a direct return is clearer than having `write` store both write 
and close errors?

```
  if (write.isSome() && close.isError()) {
return Error(...);
  }
  
  return write;
```



3rdparty/stout/include/stout/protobuf.hpp
Lines 164 (patched)


Does `stringify(*fd)` make it fit on one line?



3rdparty/stout/include/stout/protobuf.hpp
Line 197 (original), 197-203 (patched)


ditto here


- Benjamin Mahler


On Jan. 16, 2019, 1:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69082/
> ---
> 
> (Updated Jan. 16, 2019, 1:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Alex Clemmer.
> 
> 
> Bugs: MESOS-9331
> https://issues.apache.org/jira/browse/MESOS-9331
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When e.g., writing to disk, errors from write might only manifest at
> ::close time. We update some instances to correctly propagate these
> errors instead of dropping them silently. We only propagate the
> ::close error if the write operation succeeded, otherwise we just
> propagate the error from the write operation.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/mktemp.hpp 
> 63b3d1a7720d07f877fa1d4eb7f32a548916637a 
>   3rdparty/stout/include/stout/os/write.hpp 
> f7538f94f5a953a7a90a05bc1d2f138b6c17f814 
>   3rdparty/stout/include/stout/protobuf.hpp 
> eb4adef56f1701e3c101284e05e4e6c66eef9180 
> 
> 
> Diff: https://reviews.apache.org/r/69082/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69082: Correctly propagated `close` failures in some instances.

[Benjamin Mahler]

> On Jan. 15, 2019, 6:22 p.m., Benjamin Mahler wrote:
> > Should we be surfacing a close EINTR as an error or let that be silent?
> > I think these errors need some message pre-fixing? E.g.
> > 
> > ```
> > Failed to close '3': Bad file number
> > ```
> > 
> > As it stands the error messages will only show "Bad file number" and it 
> > won't be clear if the write or close produced this? It's also an issue here 
> > between open/write/flush but would be great to resolve this now.
> 
> Benjamin Bannier wrote:
> If I read the POSIX spec for `close` correctly, if `close` fails with 
> `EINTR` the passed file descriptor is left in an unspecified state so it 
> e.g., would not be safe to assume that pending data was successfully flushed. 
> Am I missing something?

There's quite a rabbit hole of reading around this, but with respect to the 
POSIX close spec:

> If close() is interrupted by a signal that is to be caught, it shall
> return -1 with errno set to [EINTR] and the state of fildes is unspecified.
> If an I/O error occurred while reading from or writing to the file system
> during close(), it may return -1 with errno set to [EIO]; if this error is
> returned, the state of fildes is unspecified.

My interpretation so far from reading more about this is that the "state of 
fildes is unspecified" is *only* referring to whether it's closed or open. 
Howerver, Linus says:

> The error return just tells you that soem error happened on the file: for
> example, in the case of EINTR, the close() may not have flushed all the
> pending data synchronously.

https://lkml.org/lkml/2005/9/10/129

But, even if close returns successfully we cannot assume pending data was 
flushed (although some of our code in question here uses an explicit flush 
before closing as suggested), from linux's close man page:

> A successful close does not guarantee that the data has been
> successfully saved to disk, as the kernel uses the buffer cache to
> defer writes.  Typically, filesystems do not flush buffers when a
> file is closed.  If you need to be sure that the data is physically
> stored on the underlying disk, use fsync(2).  (It will depend on the
> disk hardware at this point.)

Based on this, EINTR (which is more like EINPROGRESS in terms of semantics, see 
https://ewontfix.com/4/) seems to provide the same guarantee as return code of 
0:

* `0`: filedes is closed, previous `write()`s are in kernel buffer cache but 
data might not make it to disk
* `EINTR`: filedes is closed, close call was interrupted by a signal, previous 
`write()`s are in kernel buffer cache but data might not make it to disk

In the case of an `fsync()` before close, it seems to mean:

* `0`: filedes is closed, nothing in kernel buffer cache, data is already 
fsynced
* `EINTR`: filedes is closed, nothing in kernel buffer cache, data is already 
fsynced, close call was interrupted by a signal (not sure if EINTR is possible 
with an `fsync()` beforehand..)

Looking through the linux source, it calls through these:
https://github.com/torvalds/linux/blob/d7393226d15add056285c8fc86723d54d7e0c77d/fs/open.c#L1152-L1169
https://github.com/torvalds/linux/blob/903b77c631673eeec9e9114e9524171cdf9a2646/fs/file.c#L617-L641
https://github.com/torvalds/linux/blob/ad1d69735878a6bf797705b5d2a20316d35e1113/fs/open.c#L1126-L1150
and the interruptible part appears to be the flush here:
https://github.com/torvalds/linux/blob/ad1d69735878a6bf797705b5d2a20316d35e1113/fs/open.c#L1140

Trying to figure out what this flush is, I see this: 
https://lwn.net/Articles/576478/

> In fact, it is difficult to even return EINTR from close() on Linux, 
> according to Christoph Hellwig. If the driver or filesystem's release() 
> method returns an error, it is explicitly ignored. The only path that would 
> allow a driver to return EINTR is if it provides a flush() method that does 
> so. Hellwig plans to post a patch that would enforce a no-EINTR policy on 
> that path as well.

> If EINTR can never be returned, there is no real reason to map it to 
> EINPROGRESS in glibc. But, since glibc may be used on an older kernel that 
> can return EINTR in some rare situations, mapping it to something probably 
> makes sense. That could be EINPROGRESS or, perhaps better still, just zero 
> for success, as suggested by Rich Felker. There really isn't much the 
> application programmer can do if close() returns an error.

I'm not sure under which circumstances flushing is occurring on close (not sure 
which drivers provide the flush method), but this makes it sound rare / driver 
dependent, and it sounds like the plan would make EINTR impossible.

So, I guess surfacing EINTR up to the caller sounds like the simplest thing to 
do, and we'll have to see whether this actually occurs in practice.


- Benjamin


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

Re: Review Request 68019: Added a parser for the Docker Seccomp config format.

[Gilbert Song]

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




src/linux/seccomp/seccomp_parser.cpp
Lines 84-89 (patched)


Nits, I would prefer:
```
return Error(
"Cannot determine 'defaultAction' for the Seccomp configuration: " +
(defaultAction.isError() ? defaultAction.error() : "Not found");
```

ditto to all others.


- Gilbert Song


On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
>   src/linux/seccomp/seccomp_parser.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp_parser.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68019/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 68021: Added `linux/seccomp` isolator.

[Gilbert Song]

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


Fix it, then Ship it!





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


equivalent to `if(ret == -1 || EFAULT == errno)`?



src/slave/containerizer/mesos/isolators/linux/seccomp.cpp
Lines 62-63 (patched)


I would cache it and just parse the default profile once. Let's introduce a 
private variable:

Option defaultProfile;



src/slave/containerizer/mesos/isolators/linux/seccomp.cpp
Lines 95 (patched)


"Missing Seccomp profile name"



src/slave/containerizer/mesos/isolators/linux/seccomp.cpp
Lines 107 (patched)


only parse it if it is the profile from the framework API.



src/slave/containerizer/mesos/isolators/linux/seccomp.cpp
Lines 114 (patched)


kill newline


- Gilbert Song


On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68021/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9035
> https://issues.apache.org/jira/browse/MESOS-9035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `linux/seccomp` isolator which is used for
> preparing `ContainerSeccompProfile` for the Mesos containerizer
> launcher. If the `ContainerConfig` message has an info about Seccomp
> profile name, then this info will be used to locate a Seccomp profile.
> The given Seccomp profile is parsed and the resulting
> `ContainerSeccompProfile` is stored in the `ContainerLaunchInfo`
> message.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 5016f2e9f0651abcb0a5f364e8eace458f2edeae 
>   src/slave/containerizer/mesos/isolators/linux/seccomp.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/seccomp.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68021/diff/15/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 68020: Added Seccomp-related flags to the agent.

[Gilbert Song]

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


Ship it!





src/slave/flags.cpp
Lines 1397 (patched)


I would like to re-visit the naming seccomp_profile V.S. 
seccomp_profile_name. WDYT?

cc @qianzhang


- Gilbert Song


On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68020/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `--seccomp_config_dir` and `--seccomp_profile_name` flags have been
> added to the agent. These flags are used by the `linux/seccomp`
> isolator to specify the path of the directory containing Seccomp
> profiles and the name of the default Seccomp profile.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md 330283f4e3957075dd4310de4a841feac23de36c 
>   src/slave/flags.hpp 494ae02ab5eb365e2cda5017be573691107c3f28 
>   src/slave/flags.cpp 6bac8e1409f04d639204c45eda8a90c098e3dbd0 
> 
> 
> Diff: https://reviews.apache.org/r/68020/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 69790: Fixed a typo.

[Gastón Kleiman]

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

Review request for mesos, Benno Evers and Greg Mann.


Repository: mesos


Description
---

Fixed a typo.


Diffs
-

  src/slave/slave.cpp ed92f672f5155d70a36ba3619bb6f06fa09bc836 


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


Testing
---


Thanks,

Gastón Kleiman

Review Request 69792: Added an ostream operator for `Operation`.

[Gastón Kleiman]

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

Review request for mesos, Benno Evers and Greg Mann.


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


Repository: mesos


Description
---

Added an ostream operator for `Operation`.


Diffs
-

  include/mesos/type_utils.hpp 3926f491f41aee255ca13ed6a2731fbc88efb9a5 
  src/common/type_utils.cpp df888efbdb99e07584e446079c337e7b9eb7cede 


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


Testing
---

Current tests still pass.

Manually looked at the logs generated by the changes in this change, and they 
look pretty good.


Thanks,

Gastón Kleiman

Review Request 69793: Added the `ResourceState` agent protobuf message.

[Gastón Kleiman]

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

Review request for mesos, Benno Evers and Greg Mann.


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


Repository: mesos


Description
---

This protobuf message will be used by the agent to atomically checkpoint
resources and operations.


Diffs
-

  src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
  src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
  src/slave/slave.proto PRE-CREATION 


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


Testing
---

Current tests still pass.

Compiled new proto with both cmake and autotools.


Thanks,

Gastón Kleiman

Review Request 69795: Made agent recover atomically checkpointed resources and operations.

[Gastón Kleiman]

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

Review request for mesos, Benno Evers and Greg Mann.


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


Repository: mesos


Description
---

Made agent recover atomically checkpointed resources and operations.


Diffs
-

  src/slave/slave.hpp 2bcd7a93a8f25b77c71c7f931bfaac87649f987c 
  src/slave/slave.cpp ed92f672f5155d70a36ba3619bb6f06fa09bc836 
  src/slave/state.hpp e2180ae37a65a57d1edb29d2ad6cc8232029906e 
  src/slave/state.cpp ae16d6f656cff71bcbdd4ffb02888024530b9274 


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


Testing
---

Current tests still pass.


Thanks,

Gastón Kleiman

Review Request 69794: Made agent checkpoint operations affecting default resources.

[Gastón Kleiman]

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

Review request for mesos, Benno Evers and Greg Mann.


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


Repository: mesos


Description
---

This patch makes the agent atomically checkpoint resoures and operations
affecting agent default resources. This is needed in order to provide
operation feedback for operations affecting agent default resources.


Diffs
-

  src/slave/paths.hpp 73b94a8df932e40d4f266de01150bb17c2282f22 
  src/slave/paths.cpp 9fd37f5456d45d520d6062577c1692a4be627c0e 
  src/slave/slave.hpp 2bcd7a93a8f25b77c71c7f931bfaac87649f987c 
  src/slave/slave.cpp ed92f672f5155d70a36ba3619bb6f06fa09bc836 
  src/tests/slave_tests.cpp 9168e06348a83dcb20400a2fe0e3bb1f26e6ff1b 


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


Testing
---

Current tests still pass.


Thanks,

Gastón Kleiman

Review Request 69791: Made it possible to checkpoint resources without downgrading them.

[Gastón Kleiman]

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

Review request for mesos, Benno Evers and Greg Mann.


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


Repository: mesos


Description
---

For backwards compatibility the agent currently downgrades resources
before checkpointing them.

In order to support providing operation feedback for operation affecting
agent default resources, we're going to start atomically checkpoint
resources and operations in a new file.

Since we're going to checkpoint the resources to a new file that is not
used by old agents, there is no need to downgrade resources before
writing them to that file.


Diffs
-

  src/slave/state.hpp e2180ae37a65a57d1edb29d2ad6cc8232029906e 


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


Testing
---

Current tests still pass.


Thanks,

Gastón Kleiman

Re: Review Request 68018: Added `SeccompFilter` class.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Jan. 18, 2019, noon, Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> ---
> 
> (Updated Jan. 18, 2019, noon)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9034
> https://issues.apache.org/jira/browse/MESOS-9034
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `SeccompFilter` class is a wrapper for `libseccomp` API. Its main
> purpose is to provide a translation of the `ContainerSeccompProfile`
> message into calls of `libseccomp` API.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
>   src/linux/seccomp/seccomp.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68018/diff/17/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69795: Made agent recover atomically checkpointed resources and operations.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69795 was successfully built and tested.

Reviews applied: `['69790', '69791', '69792', '69793', '69794', '69795']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2804/mesos-review-69795

- Mesos Reviewbot Windows


On Jan. 18, 2019, 3 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69795/
> ---
> 
> (Updated Jan. 18, 2019, 3 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Greg Mann.
> 
> 
> Bugs: MESOS-9356
> https://issues.apache.org/jira/browse/MESOS-9356
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made agent recover atomically checkpointed resources and operations.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 2bcd7a93a8f25b77c71c7f931bfaac87649f987c 
>   src/slave/slave.cpp ed92f672f5155d70a36ba3619bb6f06fa09bc836 
>   src/slave/state.hpp e2180ae37a65a57d1edb29d2ad6cc8232029906e 
>   src/slave/state.cpp ae16d6f656cff71bcbdd4ffb02888024530b9274 
> 
> 
> Diff: https://reviews.apache.org/r/69795/diff/1/
> 
> 
> Testing
> ---
> 
> Current tests still pass.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 68019: Added a parser for the Docker Seccomp config format.

[Gilbert Song]

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


Fix it, then Ship it!





src/linux/seccomp/seccomp_parser.cpp
Lines 82 (patched)


Not a big issue but seems like in our code base we usually pass in a 
pointer instead of reference.

The positive side of passing a pointer is that it would be more explicit 
for code reader when they read the caller side of this method, they know the 
protobuf object will be mutated.

You can regard `Architecture_Parse()` as an example.

cc @bmahler



src/linux/seccomp/seccomp_parser.cpp
Lines 104 (patched)


ditto.



src/linux/seccomp/seccomp_parser.cpp
Lines 164-167 (patched)


does the order of the `architecture/subarchitecture` matter? asking cause I 
am not sure if we need architecture before its corresponding subarchitecture in 
the repeated field



src/linux/seccomp/seccomp_parser.cpp
Lines 176 (patched)


ditto.



src/linux/seccomp/seccomp_parser.cpp
Lines 263 (patched)


ditto



src/linux/seccomp/seccomp_parser.cpp
Lines 283 (patched)


index->as()?



src/linux/seccomp/seccomp_parser.cpp
Lines 290 (patched)


ditto



src/linux/seccomp/seccomp_parser.cpp
Lines 297 (patched)


ditto



src/linux/seccomp/seccomp_parser.cpp
Lines 325 (patched)


ditto.



src/linux/seccomp/seccomp_parser.cpp
Lines 490-494 (patched)


Since we somehow have to create the profile in launch.cpp, could we avoid 
create the `ctx` object twice?

Instead, for the default profile, I think we could add this verification in 
isolator::create method after we parse it


- Gilbert Song


On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
>   src/linux/seccomp/seccomp_parser.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp_parser.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68019/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69158: Added an integration test for resource provider removal.

[Benjamin Bannier]

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

(Updated Jan. 18, 2019, 10:39 a.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Addressed comments from Chun.


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


Repository: mesos


Description
---

Added an integration test for resource provider removal.


Diffs (updated)
-

  src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 


Diff: https://reviews.apache.org/r/69158/diff/8/

Changes: https://reviews.apache.org/r/69158/diff/7-8/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69615: Disable containerizer ptrace attach.

[Jiang Yan Xu]

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



For consistency if we updated command excutor we should update the default 
executor?


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


s/to to/to/



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


I haven't tried but I trust that you've verified that `PR_SET_DUMPABLE` is 
not persisted across fork but could you clarfiy via a comment? It's not clear 
from http://man7.org/linux/man-pages/man2/prctl.2.html.



src/slave/flags.cpp
Lines 338 (patched)


If disallow is the default, it's a breaking change right? Can the default 
be allowing it?


- Jiang Yan Xu


On Jan. 2, 2019, 9:15 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69615/
> ---
> 
> (Updated Jan. 2, 2019, 9:15 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9349
> https://issues.apache.org/jira/browse/MESOS-9349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use `prctl(PR_SET_DUMPABLE)` to disable the ability to attach to
> the containerizer process(es) on Linux systems. This prevents
> unprivileged containerized processes from reading information
> about the containerizer process(es) from `/proc`. This gives an
> additional layer of protection against leaking information to
> untrusted container processes.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md 330283f4e3957075dd4310de4a841feac23de36c 
>   src/launcher/executor.cpp f962e800f23d5582b1bc04a263253893492a5054 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5cf2da55c046c5c45e0c2ca3400f64de12de62b 
>   src/slave/containerizer/mesos/launch.hpp 
> 0a6394d56321948ad760ac69c05456319a254842 
>   src/slave/containerizer/mesos/launch.cpp 
> 2f1c9e7a8748c9d7eab25bc8567ca68308e680f9 
>   src/slave/flags.hpp 494ae02ab5eb365e2cda5017be573691107c3f28 
>   src/slave/flags.cpp 6bac8e1409f04d639204c45eda8a90c098e3dbd0 
>   src/slave/slave.cpp ad3b693a716cf6103345a157bf28dd60a7b07d32 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 449928c10b897061642af8ad267f8b70695940e6 
>   src/tests/slave_tests.cpp 4aed5d68e9a408821880ffaede482937be1999f4 
> 
> 
> Diff: https://reviews.apache.org/r/69615/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 29)
> 
> 
> Thanks,
> 
> James Peach
> 
>