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/#review212001
---



FAIL: Some of the unit tests failed. Please check the relevant logs.

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

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2777/mesos-review-69158/logs/mesos-tests.log):

```
I0115 07:01:03.731181 22536 master.cpp:11261] Removing task 
f96991dc-9e45-48b2-b6f6-d489f376b9ef with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 7972cd61-c070-44f2-b12d-f375b0d167d6- on 
agent 7972cd61-c070-44f2-b12d-f375b0d167d6-S0 at slave(471)@192.10.1.6:55857 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0115 07:01:03.734197 22536 master.cpp:1269] Agent 
7972cd61-c070-44f2-b12d-f375b0d167d6-S0 at slave(471)@192.10.1.6:55857 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0115 07:01:03.734197 22536 master.cpp:3272] Disconnecting agent 
7972cd61-c070-44f2-b12d-f375b0d167d6-S0 at slave(471)@192.10.1.6:55857 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0115 07:01:03.734197 22536 master.cpp:3291] Deactivating agent 
7972cd61-c070-44f2-b12d-f375b0d167d6-S0 at slave(471)@192.10.1.6:55857 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0115 07:01:03.735185 22808 hierarchical.cpp:358] Removed framework 
7972cd61-c070-44f2-b12d-f375b0d167d6-
I0115 07:01:03.735185 22808 hierarchical.cpp:802] Agent 
7972cd61-c070-44f2-b12d-f375b0d167d6-S0 deactivated
I0115 07:01:03.736189 17752 containerizer.cpp:2469] Destroying container 
26fb0c01-af27-4d31-b9ce-8294baba13ea in RUNNING state
I0115 07:01:03.736189 17752 containerizer.cpp:3136] Transitioning the state of 
container 26fb0c01-af27-4d31-b9ce-8294b[   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (590 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (606 ms total)

[--] Global test environment tear-down
[==] 1088 tests from 104 test cases ran. (501935 ms total)
[  PASSED  ] 1087 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ContentType/MasterAPITest.OperationUpdatesUponAgentGone/0, where 
GetParam() = application/x-protobuf

 1 FAILED TEST
  YOU HAVE 231 DISABLED TESTS

aba13ea from RUNNING to DESTROYING
I0115 07:01:03.736189 17752 launcher.cpp:161] Asked to destroy container 
26fb0c01-af27-4d31-b9ce-8294baba13ea
W0115 07:01:03.737174 24084 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=11992 to peer '192.10.1.6:57736': IO failed with error 
code: The specified network name is no longer available.

W0115 07:01:03.737174 24084 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=12044 to peer '192.10.1.6:57737': IO failed with error 
code: The specified network name is no longer available.

I0115 07:01:03.743180 22536 containerizer.cpp:2975] Container 
26fb0c01-af27-4d31-b9ce-8294baba13ea has exited
I0115 07:01:03.771181  2236 master.cpp:1109] Master terminating
I0115 07:01:03.773178 20280 hierarchical.cpp:644] Removed agent 
7972cd61-c070-44f2-b12d-f375b0d167d6-S0
I0115 07:01:04.118175 24084 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On Oct. 25, 2018, 6:47 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Oct. 25, 2018, 6:47 p.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 1e918712d73d0a03c4c15b41babff1f649ba9c07 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68017: Added Seccomp-related protobuf messages.

[Gilbert Song]

> On Jan. 14, 2019, 6:39 p.m., Gilbert Song wrote:
> > include/mesos/mesos.proto
> > Lines 3158-3159 (patched)
> > 
> >
> > Seems like this was added recently.
> > 
> > Is this field only used when there is a default agent wide seccomp 
> > profile provided from the agent flag? and we reply on it to give an 
> > opportunity for user/framework to get rid of seccomp?
> > 
> > (probably more comment needed)
> > 
> > If it is the case, do we have other options for naming? e.g., 
> > `no_seccomp` (maybe more explicit)
> 
> Gilbert Song wrote:
> after second thought, I would suggest to remove this field for now. since 
> there is not use case yet (I understand your motivation: you want users could 
> get rid of seccomp if there is a default one). We could add this field later 
> if necessary. For now:
> 
> two options:
> 1. remove it
> 2. do it implicitly by using the optional profile_name: if seccompinfo 
> isSome but profile_name is None. do not set seccomp filtering for container

I would prefer option #2.

The reason we want to avoid introducing `unconfined` now is that framework 
could set both field at the same time and ideally we may need an `enum type`. 
given the use case is not clear yet (people may not necessary to use it yet). 
lets make it implicit for now.


- Gilbert


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


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/68017/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9033
> https://issues.apache.org/jira/browse/MESOS-9033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 2ef6ba3aef67cf34227569948fd3ddc8dfd5879d 
>   include/mesos/seccomp/seccomp.hpp PRE-CREATION 
>   include/mesos/seccomp/seccomp.proto PRE-CREATION 
>   include/mesos/slave/containerizer.proto 
> 5b4dcdda0f55ea3355c78d1447c7be9ca54d9dc9 
>   include/mesos/v1/mesos.proto 1a701da65f653fe4191f92ff1fb1436809b50acb 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am 188a47017221a931d8b965a4af5e033b77e6ce4e 
> 
> 
> Diff: https://reviews.apache.org/r/68017/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Chun-Hung Hsiao]

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




src/tests/slave_tests.cpp
Lines 10609 (patched)


Maybe move this to 
`AgentAPITest.OperationStatusUpdateUponResourceProviderGone`? We have 
`MasterAPITest.TaskUpdatesUponAgentGone` there.


- Chun-Hung Hsiao


On Oct. 25, 2018, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Oct. 25, 2018, 10:47 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 1e918712d73d0a03c4c15b41babff1f649ba9c07 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Chun-Hung Hsiao]

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




src/tests/slave_tests.cpp
Lines 10617 (patched)


We could rename it to `updateSlaveMessage` and reuse it below.



src/tests/slave_tests.cpp
Lines 10625-10626 (patched)


```
Owned detector = master.get()->createDetector();
Try> slave = StartSlave(detector.get(), slavFlags);
```



src/tests/slave_tests.cpp
Lines 10633 (patched)


`org.apache.mesos.rp.test`, or maybe `org.apache.mesos.rp.mock`, for 
consistency.



src/tests/slave_tests.cpp
Lines 10652 (patched)


`std::move(endpointDetector)`


- Chun-Hung Hsiao


On Oct. 25, 2018, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Oct. 25, 2018, 10:47 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 1e918712d73d0a03c4c15b41babff1f649ba9c07 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68147: Added agent support to remove local resource providers.

[Chun-Hung Hsiao]

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




src/tests/api_tests.cpp
Lines 7857 (patched)


```
Owned detector = master.get()->createDetector();
```
And
```
Try> slave = StartSlave(detector.get(), slavFlags);
```
below.


- Chun-Hung Hsiao


On Oct. 25, 2018, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68147/
> ---
> 
> (Updated Oct. 25, 2018, 10:47 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for triggering permanent removal of local
> resource providers. We also add authorization and tests as part of this
> patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
>   src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a 
>   src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2 
>   src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
>   src/slave/http.cpp ba389263ef7ab311682d542424eb56360841d24c 
>   src/slave/slave.hpp edf7269d4057ec8c95bb54c855210ad00d002a50 
>   src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb 
>   src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
>   src/tests/api_tests.cpp edde48a4b4b46c9f47bc44de389c97b77322b8e8 
> 
> 
> Diff: https://reviews.apache.org/r/68147/diff/12/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68147: Added agent support to remove local resource providers.

[Chun-Hung Hsiao]

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




src/master/master.cpp
Lines 11248 (patched)


Can you add a comment explaining why it is not necessary to call 
`allocator->recoverResources(...)` here, and that the 
`slave->recoverResources(...)` and `framework->recoverResources(...)` below are 
unrelated and thus the bookkeeping would still be consistent even if 
`allocator->recoverResources(...)` is not called?



src/slave/http.cpp
Lines 3310 (patched)


Please add the following log line here:
```
LOG(INFO)
  << "Processing MARK_RESOURCE_PROVIDER_GONE for resource provider "
  << resourceProviderId;
```



src/tests/api_tests.cpp
Lines 7877 (patched)


This is not necessary since the `AWAIT_READY` below enforces the proper 
synchronization.



src/tests/api_tests.cpp
Lines 7905 (patched)


`std::move(endpointDetector)`


- Chun-Hung Hsiao


On Oct. 25, 2018, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68147/
> ---
> 
> (Updated Oct. 25, 2018, 10:47 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for triggering permanent removal of local
> resource providers. We also add authorization and tests as part of this
> patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
>   src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a 
>   src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2 
>   src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
>   src/slave/http.cpp ba389263ef7ab311682d542424eb56360841d24c 
>   src/slave/slave.hpp edf7269d4057ec8c95bb54c855210ad00d002a50 
>   src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb 
>   src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
>   src/tests/api_tests.cpp edde48a4b4b46c9f47bc44de389c97b77322b8e8 
> 
> 
> Diff: https://reviews.apache.org/r/68147/diff/12/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68147: Added agent support to remove local resource providers.

[Chun-Hung Hsiao]

> On Aug. 17, 2018, 10:29 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8195 (patched)
> > 
> >
> > Let's validate that there is no task using the resources provided by 
> > this RP before doing the removal, or fail the call.
> 
> Benjamin Bannier wrote:
> I believe this should be treated as orthogonal issue. It might e.g., 
> happen that the user triggering the RP removal cannot kill tasks or prevent 
> them from being rescheduled. I'd suggest to leaves as is.
> 
> WDYT?
> 
> Chun-Hung Hsiao wrote:
> The scenario I am worried about is that when the RP is marked as gone, 
> the RP may fail to tear itself down if there is any task using its resources.
> 
> Benjamin Bannier wrote:
> We don't seem to have a good story on incompatible resource changes ATM. 
> If we e.g., update a RP config in an incompatible way, we make not attempt to 
> detect or rectify this in any way, either. At least in the agent we were in 
> general we were operating under the assumption that tasks would die on 
> incompatible changes to their resources.
> 
> I'd suggest to drop this issue for now and potentially file a JIRA to 
> track draining and situations where it might be needed.
> 
> Benjamin Bannier wrote:
> Dropping this, see above comment.

Reopening this. Here's my suggestion:

1. Create a ticket for a proper design to drain the resource provider.
2. For now, let's fail the call here if 
`slave->resourceProviders.at(resourceProviderId)->totalResources` is not empty. 
In other words, the operator must first reconfigure the resource provider to 
"drain" itself before making the `MARK_RESOURCE_PROVIDER_GONE` call.


- Chun-Hung


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


On Oct. 25, 2018, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68147/
> ---
> 
> (Updated Oct. 25, 2018, 10:47 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for triggering permanent removal of local
> resource providers. We also add authorization and tests as part of this
> patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
>   src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a 
>   src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2 
>   src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
>   src/slave/http.cpp ba389263ef7ab311682d542424eb56360841d24c 
>   src/slave/slave.hpp edf7269d4057ec8c95bb54c855210ad00d002a50 
>   src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb 
>   src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
>   src/tests/api_tests.cpp edde48a4b4b46c9f47bc44de389c97b77322b8e8 
> 
> 
> Diff: https://reviews.apache.org/r/68147/diff/12/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68017: Added Seccomp-related protobuf messages.

[Gilbert Song]

> On Jan. 14, 2019, 6:39 p.m., Gilbert Song wrote:
> > include/mesos/mesos.proto
> > Lines 3158-3159 (patched)
> > 
> >
> > Seems like this was added recently.
> > 
> > Is this field only used when there is a default agent wide seccomp 
> > profile provided from the agent flag? and we reply on it to give an 
> > opportunity for user/framework to get rid of seccomp?
> > 
> > (probably more comment needed)
> > 
> > If it is the case, do we have other options for naming? e.g., 
> > `no_seccomp` (maybe more explicit)

after second thought, I would suggest to remove this field for now. since there 
is not use case yet (I understand your motivation: you want users could get rid 
of seccomp if there is a default one). We could add this field later if 
necessary. For now:

two options:
1. remove it
2. do it implicitly by using the optional profile_name: if seccompinfo isSome 
but profile_name is None. do not set seccomp filtering for container


- Gilbert


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


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/68017/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9033
> https://issues.apache.org/jira/browse/MESOS-9033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 2ef6ba3aef67cf34227569948fd3ddc8dfd5879d 
>   include/mesos/seccomp/seccomp.hpp PRE-CREATION 
>   include/mesos/seccomp/seccomp.proto PRE-CREATION 
>   include/mesos/slave/containerizer.proto 
> 5b4dcdda0f55ea3355c78d1447c7be9ca54d9dc9 
>   include/mesos/v1/mesos.proto 1a701da65f653fe4191f92ff1fb1436809b50acb 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am 188a47017221a931d8b965a4af5e033b77e6ce4e 
> 
> 
> Diff: https://reviews.apache.org/r/68017/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 68022: Enabled Seccomp filter in the containerizer launcher.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Aug. 6, 2018, 9:39 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68022/
> ---
> 
> (Updated Aug. 6, 2018, 9:39 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9106
> https://issues.apache.org/jira/browse/MESOS-9106
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Containerizer launcher creates an instance of `SeccompFilter`, which is
> used to setup Seccomp profile using `ContainerSeccompProfile` message
> prepared by the `linux/seccomp` isolator. The Seccomp filter is loaded
> right before calling `execve()`, so that a container will be running
> with a syscall filtering enabled.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 2f1c9e7a8748c9d7eab25bc8567ca68308e680f9 
> 
> 
> Diff: https://reviews.apache.org/r/68022/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/linux/seccomp.cpp
Lines 17-18 (patched)


A newline between.



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


Can we have a more explicit error message here? Like `The default seccomp 
profile is invalid: ...`.



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


Missing Seccomp profile name for container xxx.



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


I do not think we need `Option` here.


- Qian Zhang


On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68021/
> ---
> 
> (Updated Nov. 8, 2018, 11:24 p.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 bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5cf2da55c046c5c45e0c2ca3400f64de12de62b 
>   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/12/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Gilbert Song]

> On Jan. 2, 2019, 5:58 p.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 141-144 (patched)
> > 
> >
> > Instead of always set `SCMP_FLTATR_CTL_NNP`. Should we consider to 
> > check root privileges first (e.g., `geteuid() != 0`)?
> 
> Andrei Budnik wrote:
> By default, libseccomp sets `true` to the `SCMP_FLTATR_CTL_NNP` flag
> 
> https://github.com/seccomp/libseccomp/blob/1e64feb5f1a9ea02687228e3073e8b784a04ce46/src/db.c#L960
> 
> Hence, all Seccomp test pass even after removing `seccomp_attr_set(ctx, 
> SCMP_FLTATR_CTL_NNP, 1)`. Also, this means that Docker daemon launches its 
> containers with this flag set by default (as they also use libseccomp).
> 
> Disabling `SCMP_FLTATR_CTL_NNP` flag for a `root` means that Seccomp 
> filter can be reverted anytime. So, disabling this flag is meaningless.

Gotcha. Does it imply that task launched by docker daemon with seccomp profile 
enabled cannot setuid (assuming docker relies on execve/execvpe)?


> On Jan. 2, 2019, 5:58 p.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 147 (patched)
> > 
> >
> > Could we use `foreach (const ContainerSeccompProfile::Architecture& 
> > arch, profile.architectures())`?
> > 
> > So that it avoids the implicit conversion to `int` and also avoid the 
> > `static_cast` below?
> 
> Andrei Budnik wrote:
> I've already replied to the same comment above from Qian.
> 
> The return type of `architectures()` is `const 
> ::google::protobuf::RepeatedField&`, so it won't compile:
> 
> `../../src/linux/seccomp/seccomp.cpp:147:85: error: invalid conversion 
> from ‘int’ to ‘mesos::slave::ContainerSeccompProfile::Architecture {aka 
> mesos::slave::ContainerSeccompProfile_Architecture}’ [-fpermissive]`

Just checked the .pb.h file. Good to know, thanks!


- Gilbert


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


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/68018/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> 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/15/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 68017: Added Seccomp-related protobuf messages.

[Gilbert Song]

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


Fix it, then Ship it!





include/mesos/mesos.proto
Lines 3158-3159 (patched)


Seems like this was added recently.

Is this field only used when there is a default agent wide seccomp profile 
provided from the agent flag? and we reply on it to give an opportunity for 
user/framework to get rid of seccomp?

(probably more comment needed)

If it is the case, do we have other options for naming? e.g., `no_seccomp` 
(maybe more explicit)


- 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/68017/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9033
> https://issues.apache.org/jira/browse/MESOS-9033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 2ef6ba3aef67cf34227569948fd3ddc8dfd5879d 
>   include/mesos/seccomp/seccomp.hpp PRE-CREATION 
>   include/mesos/seccomp/seccomp.proto PRE-CREATION 
>   include/mesos/slave/containerizer.proto 
> 5b4dcdda0f55ea3355c78d1447c7be9ca54d9dc9 
>   include/mesos/v1/mesos.proto 1a701da65f653fe4191f92ff1fb1436809b50acb 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am 188a47017221a931d8b965a4af5e033b77e6ce4e 
> 
> 
> Diff: https://reviews.apache.org/r/68017/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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/#review211981
---


Fix it, then Ship it!





src/linux/seccomp/seccomp.cpp
Lines 173 (patched)


nits:

Added Seccomp architecture ...


- 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/68018/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> 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/15/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Qian Zhang]

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


Fix it, then Ship it!





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


Path or name?


- Qian Zhang


On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68020/
> ---
> 
> (Updated Nov. 8, 2018, 11:24 p.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
> 
>

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

[Gilbert Song]

> On Jan. 14, 2019, 12:31 a.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 137-139 (patched)
> > 
> >
> > Will this affect the task run by Mesos? E.g., a task may want to run a 
> > program which has `set-user-ID` bit.
> 
> Andrei Budnik wrote:
> Yes, `no_new_privs` flag affects the task that wants to run a program 
> which has `set-user-ID` bit.
> E.g., launching a `ping -c 3 8.8.8.8` fails with seccomp. You'll see a 
> message in executor logs:
> ```
> I0114 07:19:21.887670 13264 executor.cpp:706] Forked command at 13276
> ping: socket: Operation not permitted
> I0114 07:19:22.055352 13263 executor.cpp:1007] Command exited with status 
> 2 (pid: 13276)
> ```
> 
> Also, see my previous comment 
> https://reviews.apache.org/r/68018/#comment297000
> 
> Qian Zhang wrote:
> In your previous comment, you mentioned that Docker daemon launches its 
> containers with `SCMP_FLTATR_CTL_NNP` flag set by default, does that mean any 
> containers launched by Docker daemon cannot run program which has set-user-ID 
> bit?
> 
> This seems unfortunate since it might break some use cases or 
> applications that we already supported. And can you please elaborate a bit 
> about `"Disabling SCMP_FLTATR_CTL_NNP flag for a root means that Seccomp 
> filter can be reverted anytime"`? How will the Seccomp filter be reverted? Do 
> you mean the task launched by Mesos can call libseccomp API to revert the 
> filter itself?
> 
> If we have to live with this limitation (i.e., cannot run program which 
> has set-user-ID bit), then we need to highlight it in the document.

Seems like we asked the same question.

Andrei, let align on this thread? :/thanks:)


- Gilbert


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


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/68018/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> 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/15/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Qian Zhang]

> On Jan. 14, 2019, 4:31 p.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 137-139 (patched)
> > 
> >
> > Will this affect the task run by Mesos? E.g., a task may want to run a 
> > program which has `set-user-ID` bit.
> 
> Andrei Budnik wrote:
> Yes, `no_new_privs` flag affects the task that wants to run a program 
> which has `set-user-ID` bit.
> E.g., launching a `ping -c 3 8.8.8.8` fails with seccomp. You'll see a 
> message in executor logs:
> ```
> I0114 07:19:21.887670 13264 executor.cpp:706] Forked command at 13276
> ping: socket: Operation not permitted
> I0114 07:19:22.055352 13263 executor.cpp:1007] Command exited with status 
> 2 (pid: 13276)
> ```
> 
> Also, see my previous comment 
> https://reviews.apache.org/r/68018/#comment297000

In your previous comment, you mentioned that Docker daemon launches its 
containers with `SCMP_FLTATR_CTL_NNP` flag set by default, does that mean any 
containers launched by Docker daemon cannot run program which has set-user-ID 
bit?

This seems unfortunate since it might break some use cases or applications that 
we already supported. And can you please elaborate a bit about `"Disabling 
SCMP_FLTATR_CTL_NNP flag for a root means that Seccomp filter can be reverted 
anytime"`? How will the Seccomp filter be reverted? Do you mean the task 
launched by Mesos can call libseccomp API to revert the filter itself?

If we have to live with this limitation (i.e., cannot run program which has 
set-user-ID bit), then we need to highlight it in the document.


- Qian


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


On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> ---
> 
> (Updated Nov. 8, 2018, 11:24 p.m.)
> 
> 
> 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/15/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69658: Avoided manual indexing during iteration.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69658 was successfully built and tested.

Reviews applied: `['69658']`

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

- Mesos Reviewbot Windows


On Jan. 3, 2019, 9:06 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69658/
> ---
> 
> (Updated Jan. 3, 2019, 9:06 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch replaces an instance of manual indexing during iteration with
> a `foreach`. We also remove an unused variable.
> 
> This is a non-functional change.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
> 
> 
> Diff: https://reviews.apache.org/r/69658/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69669: Notified frameworks when operations are marked as unreachable.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Jan. 11, 2019, 2:24 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69669/
> ---
> 
> (Updated Jan. 11, 2019, 2:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8783
> https://issues.apache.org/jira/browse/MESOS-8783
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent is being marked as unreachable due to missing
> the reregistration timeout, all operations on that agent
> are implicilty transitioned to status `OPERATION_UNREACHABLE`.
> 
> This commit adds an explicit notification for this transition
> to frameworks which opted-in to operation feedback.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 8de73124dbfb81d6edd0d1d5193adc21756f3fad 
>   src/master/master.cpp 9624832d017dae717da803ca2ff2f8fc5135ea9d 
>   src/tests/api_tests.cpp c597243e2e210e83a4ab7441fbcfa3198b43d849 
> 
> 
> Diff: https://reviews.apache.org/r/69669/diff/3/
> 
> 
> Testing
> ---
> 
> Internal CI run.
> 
> `./src/mesos-tests --gtest_filter="*OperationUpdatesUponUnreachable*" 
> --verbose --gtest_repeat=5000`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Lines 3357 (patched)


```
// ... a failed, non-restartable state (see MESOS-9130).
```


- Chun-Hung Hsiao


On Jan. 7, 2019, 4:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69687/
> ---
> 
> (Updated Jan. 7, 2019, 4:19 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Plugin restarts can currently fail if the resource provider is killed
> before it has performed minimal reconciliation steps. This patch adds
> additional synchronization to ensure the plugin container can safely be
> killed to safely perform the desired test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69687/diff/1/
> 
> 
> Testing
> ---
> 
> * `make check`
> * ran test under stress for ~1000 iterations w/o issues
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69658: Avoided manual indexing during iteration.

[Chun-Hung Hsiao]

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




src/resource_provider/storage/provider.cpp
Line 1211 (original), 1211 (patched)


This does not look right to me. The intention of the manual indexing is to 
skip all operation statuses that the status update manager already knows about.


- Chun-Hung Hsiao


On Jan. 3, 2019, 9:06 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69658/
> ---
> 
> (Updated Jan. 3, 2019, 9:06 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch replaces an instance of manual indexing during iteration with
> a `foreach`. We also remove an unused variable.
> 
> This is a non-functional change.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
> 
> 
> Diff: https://reviews.apache.org/r/69658/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69719: Exposed subscriptions and disconnections RP manager metrics.

[Chun-Hung Hsiao]

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




src/resource_provider/manager.cpp
Lines 823-832 (original), 827-836 (patched)


How about moving this snippt to the end of this function, so the agent 
would receive `SUBSCRIBE` and then `DISCONNECT` no matter if the RP has a 
connection problem after it subscribes? This would also make the metric reveal 
the connection problem.



src/resource_provider/manager.cpp
Lines 1004-1005 (patched)


How about the following, to make it consistent with, e.g., 
`master/messages_register_framework`, and also make it less confusing w/ 
`resource_provider_manager/subscribed`?
```
messages_subscribe("resource_provider_manager/messages_subscribe"),
messages_disconnect("resource_provider_manager/messages_disconnect")
```



src/tests/resource_provider_manager_tests.cpp
Lines 1463-1468 (patched)


We usually do the following instead:
```
ASSERT_NE(0u, snapshot.values.count(...));
```
Or
```
ASSERT_EQ(1u, snapshot.values.count(...));
```

But I have to admit that `ASSERT_TRUE` is more readable. So it's up to you 
to decide if you want to keep the consistency or not ;)



src/tests/resource_provider_manager_tests.cpp
Lines 1470 (patched)


Maybe move all assertions related to a specific metric together? E.g.,
```
ASSERT_TRUE(snapshot.values.count(".../subscribed"));
EXPECT_EQ(0, snapshot.values.at(".../subscribed"));
ASSERT_TRUE(snapshot.values.count(".../subscriptions"));
EXPECT_EQ(0, snapshot.values.at(".../subscriptions"));
...
```



src/tests/resource_provider_manager_tests.cpp
Lines 1507-1509 (patched)


Shouldn't we test the existence of the metric keys, since this is a newly 
generated snapshot?


- Chun-Hung Hsiao


On Jan. 11, 2019, 10:02 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69719/
> ---
> 
> (Updated Jan. 11, 2019, 10:02 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and James DeFelice.
> 
> 
> Bugs: MESOS-9223
> https://issues.apache.org/jira/browse/MESOS-9223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds monotonically increasing counters for subscriptions and
> disconnections of resource providers with the resource provider manager
> (`resource_provider_manager/subscriptions` and
> `resource_provider_manager/disconnections`, respectively). While the
> existing gauge `resource_provider_manager/subscribed` exposed the
> current state, these added counters can be used to monitor resource
> provider state for unexpected events.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 65852c629393f32fd582bfcff86d7ce14e5386ac 
>   src/tests/resource_provider_manager_tests.cpp 
> 20cfb340bf634354865d79c92ee70cddca08f28a 
> 
> 
> Diff: https://reviews.apache.org/r/69719/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68017: Added Seccomp-related protobuf messages.

[Gilbert Song]

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




include/mesos/seccomp/seccomp.proto
Lines 21-28 (patched)


This is my fault. Sorry, Andrei!

Would you mind moving this .proto to /src/seccomp again? The previous 
suggestion was not appropriate after the second thought because:

proto files under /include:
They are public protobuf files that may be used by external/3th party 
modules, while some of the proto files like v1/v2.proto and fetcher.proto 
should be moved out from /include dir. this is a tech debt.

proto files under /src:
used for internal conversion.


- 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/68017/
> ---
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9033
> https://issues.apache.org/jira/browse/MESOS-9033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 2ef6ba3aef67cf34227569948fd3ddc8dfd5879d 
>   include/mesos/seccomp/seccomp.hpp PRE-CREATION 
>   include/mesos/seccomp/seccomp.proto PRE-CREATION 
>   include/mesos/slave/containerizer.proto 
> 5b4dcdda0f55ea3355c78d1447c7be9ca54d9dc9 
>   include/mesos/v1/mesos.proto 1a701da65f653fe4191f92ff1fb1436809b50acb 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am 188a47017221a931d8b965a4af5e033b77e6ce4e 
> 
> 
> Diff: https://reviews.apache.org/r/68017/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69721: Broke up `SSLTest.ProtocolMismatch` into smaller tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69721 was successfully built and tested.

Reviews applied: `['69720', '69721']`

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

- Mesos Reviewbot Windows


On Jan. 14, 2019, 7:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69721/
> ---
> 
> (Updated Jan. 14, 2019, 7:54 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-5189
> https://issues.apache.org/jira/browse/MESOS-5189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch breaks up the different iterations in
> `SSLTest.ProtocolMismatch` into smaller tests
> `SSLProtocol/SSLProtocolTest.Mismatch/N`. While this slightly
> increases the time to run all combinations sequentially, it leads to
> much shorter aggregated run time in parallel test execution.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> bfb5eab255230b7fbfdefaa62be27a8f8c4e9517 
> 
> 
> Diff: https://reviews.apache.org/r/69721/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69708: Fixed gRPC CMake build issue on Ubuntu 14.04.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69708 was successfully built and tested.

Reviews applied: `['69725', '69708']`

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

- Mesos Reviewbot Windows


On Jan. 14, 2019, 1:47 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69708/
> ---
> 
> (Updated Jan. 14, 2019, 1:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, BenjaminVW BenjaminVW, Gastón 
> Kleiman, Joseph Wu, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-9519
> https://issues.apache.org/jira/browse/MESOS-9519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch cherry-picks an unofficial commit to disable ALPN compilation
> if only OpenSSL 1.0.1 is on the system.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 703808d063e4bba58f647b5d48b78724003bcc4e 
>   3rdparty/grpc-1.10.0.patch 655f00387a6b308b653b23053419ec05c8b22144 
>   3rdparty/grpc.md e06843c8b6038eb9fb809241686fd611d1daedc8 
> 
> 
> Diff: https://reviews.apache.org/r/69708/diff/5/
> 
> 
> Testing
> ---
> 
> OS=ubuntu:14.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
> --enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
> support/docker-build.sh
> OS=ubuntu:16.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
> --enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
> support/docker-build.sh (to make sure that it works with OpenSSL 1.0.2. Also 
> manually inspected that ALPN symbols are linked).
> Manually tested on Debian 9 to ensure that the CMake rule can find its 
> OpenSSL 1.1.0.
> 
> NOTE: The patch itself does not depend on r/69725, but the ubuntu:16.04 test 
> above does.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69606: Made starting and stopping on CSI plugin containers more verbose.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!




Not sure how much we gain from this patch, since the container ID, which is 
printed out by the container daemon before invoking the post-start/stop hooks, 
already has the type and name of the CSI plugin.


src/resource_provider/storage/provider.cpp
Lines 2021-2024 (patched)


Since it is possible for a CSI plugin to run two containers, one for the 
controller service and one for the node service, how about the following?
```
LOG(INFO)
  << "CSI plugin container '" << containerId << "' started for plugin type 
'"
  << info.storage().plugin().type() << "' and name '"
  << info.storage().plugin().name() << "'";
```

It's also more consistent with how type-name pairs are logged elsewhere.

Ditto below.


- Chun-Hung Hsiao


On Dec. 21, 2018, 2:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69606/
> ---
> 
> (Updated Dec. 21, 2018, 2:03 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9223
> https://issues.apache.org/jira/browse/MESOS-9223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds some additional logging so it becomes easier to follow
> the lifecycle of CSI plugins. The container daemon already logged some
> related information, but didn't directly call out that it was working
> with CSI plugin containers.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> a0641665710e08f6f3b4b3ece7bcc5c6d4ef 
> 
> 
> Diff: https://reviews.apache.org/r/69606/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69708: Fixed gRPC CMake build issue on Ubuntu 14.04.

[Chun-Hung Hsiao]

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

(Updated Jan. 14, 2019, 9:47 p.m.)


Review request for mesos, Benjamin Bannier, BenjaminVW BenjaminVW, Gastón 
Kleiman, Joseph Wu, Till Toenshoff, and Vinod Kone.


Changes
---

Formatted the patch with `git format-patch --stdout -N -p v1.10.0`.


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


Repository: mesos


Description
---

This patch cherry-picks an unofficial commit to disable ALPN compilation
if only OpenSSL 1.0.1 is on the system.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 703808d063e4bba58f647b5d48b78724003bcc4e 
  3rdparty/grpc-1.10.0.patch 655f00387a6b308b653b23053419ec05c8b22144 
  3rdparty/grpc.md e06843c8b6038eb9fb809241686fd611d1daedc8 


Diff: https://reviews.apache.org/r/69708/diff/5/

Changes: https://reviews.apache.org/r/69708/diff/4-5/


Testing
---

OS=ubuntu:14.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
--enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
support/docker-build.sh
OS=ubuntu:16.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
--enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
support/docker-build.sh (to make sure that it works with OpenSSL 1.0.2. Also 
manually inspected that ALPN symbols are linked).
Manually tested on Debian 9 to ensure that the CMake rule can find its OpenSSL 
1.1.0.

NOTE: The patch itself does not depend on r/69725, but the ubuntu:16.04 test 
above does.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69656: Fixed test `MesosContainerizerSlaveRecoveryTest.ResourceStatistics`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69656 was successfully built and tested.

Reviews applied: `['69656']`

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

- Mesos Reviewbot Windows


On Jan. 3, 2019, 5:16 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69656/
> ---
> 
> (Updated Jan. 3, 2019, 5:16 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Bugs: MESOS-5048
> https://issues.apache.org/jira/browse/MESOS-5048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is flaky
> due to a race between executor shutdown (due to never getting any
> tasks) and the test querying resource statistics. If the executor
> is shutdown before the statistics query, the test will fail.
> 
> This patch fixes the test by explicitly waiting for the task to
> be delivered and task status transition to `TASK_RUNNING` before
> restarting the agent. This way, the executor will not be shutdown
> after agent restart. Hence there will be no race.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 0eb47e2bdf6a46fc21b59bb85b4b89181087ccd3 
> 
> 
> Diff: https://reviews.apache.org/r/69656/diff/1/
> 
> 
> Testing
> ---
> 
> ran `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` continuously 
> without failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69708: Fixed gRPC CMake build issue on Ubuntu 14.04.

[Chun-Hung Hsiao]

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

(Updated Jan. 14, 2019, 8:21 p.m.)


Review request for mesos, Benjamin Bannier, BenjaminVW BenjaminVW, Gastón 
Kleiman, Joseph Wu, Till Toenshoff, and Vinod Kone.


Changes
---

Addressed Joseph's comment.


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


Repository: mesos


Description
---

This patch cherry-picks an unofficial commit to disable ALPN compilation
if only OpenSSL 1.0.1 is on the system.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 703808d063e4bba58f647b5d48b78724003bcc4e 
  3rdparty/grpc-1.10.0.patch 655f00387a6b308b653b23053419ec05c8b22144 
  3rdparty/grpc.md e06843c8b6038eb9fb809241686fd611d1daedc8 


Diff: https://reviews.apache.org/r/69708/diff/4/

Changes: https://reviews.apache.org/r/69708/diff/3-4/


Testing
---

OS=ubuntu:14.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
--enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
support/docker-build.sh
OS=ubuntu:16.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
--enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
support/docker-build.sh (to make sure that it works with OpenSSL 1.0.2. Also 
manually inspected that ALPN symbols are linked).
Manually tested on Debian 9 to ensure that the CMake rule can find its OpenSSL 
1.1.0.

NOTE: The patch itself does not depend on r/69725, but the ubuntu:16.04 test 
above does.


Thanks,

Chun-Hung Hsiao

[GitHub] kaysoky commented on a change in pull request #324: MESOS-9499 extended URI syntax to support any Zookeeper authentication schemes

[GitBox]

kaysoky commented on a change in pull request #324: MESOS-9499 extended URI 
syntax to support any Zookeeper authentication schemes
URL: https://github.com/apache/mesos/pull/324#discussion_r247638473
 
 

 ##
 File path: include/mesos/zookeeper/url.hpp
 ##
 @@ -101,16 +102,35 @@ inline Try URL::parse(const std::string& url)
   // Look for the trailing '@' (if any), that's where servers starts.
   size_t index = s.find_last_of('@');
 
-  if (index != std::string::npos) {
-return URL(s.substr(0, index), s.substr(index + 1), path);
-  } else {
+  if (index == std::string::npos)
 return URL(s, path);
+
+  std::string servers = s.substr(index + 1);
+  std::string auth = s.substr(0, index);
+
+  size_t schemeDelimiter = auth.find_first_of('!');
+
+  // If there is not '!' in URL scheme is "digest" and everything before '@' 
is credentials
+  std::string scheme = "digest";
+  std::string credentials = auth;
 
 Review comment:
   Adding the authentication scheme into this location gives us a small-ish 
patch, but has downsides:
   
   - The addition of an exclamation mark delimiter invalidates the URI schema, 
according to https://tools.ietf.org/html/rfc1034#section-3.5 .  Only letters, 
digits, and dashes are allowed in a "host" field.  If we (Mesos) ever change 
our URI parsing logic, we would need to rip out this code.
   - Zookeeper's pluggable system does not seem to limit the characters in an 
authentication scheme (as far as I know).  If a plugin's scheme has an `@` in 
the name for whatever reason, the URI parsing logic here would fail 
spectacularly  .
   
   My recommendation is to add an optional master/agent flag which specifies 
the ZK authentication scheme.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 69729: Removed unused class fields in the allocator.

[Meng Zhu]

> On Jan. 11, 2019, 6:15 p.m., Benjamin Mahler wrote:
> > Seems useful to add a bit of context to the description that these are now 
> > duplicated via the options struct and used through that? Did I understand 
> > right?

That is correct. Updated the summary.


- Meng


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


On Jan. 11, 2019, 4:12 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69729/
> ---
> 
> (Updated Jan. 11, 2019, 4:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These fields were used before we introducing the
> `mesos::allocator::Options` struct. They are now
> duplicated and unused.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b58371b5247c6a479b10b6967ef852801081e6e8 
>   src/master/allocator/mesos/hierarchical.hpp 
> a4425bc952864d6a0dacee4ae92637fd31e4a45f 
> 
> 
> Diff: https://reviews.apache.org/r/69729/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69720: Made `SSLTest` an unparameterized test suite.

[Benjamin Bannier]

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

(Updated Jan. 14, 2019, 8:54 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Addressed Joseph's comments.


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


Repository: mesos


Description
---

This general and reusable testing class is made unparameterized since
deriving from a parameterized test is cumbersome (e.g., requires manual
disambiguation of possibly private types and methods inherited from the
respective `::testing::WithParamInterface` base class).

This patch makes `SSLTest` an unparameterized test suite which eases
reuse. We are able to remove a number of redundant test
parameterizations along the way.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
23d7aee963b6fb489403a94500d39e3413c7fcdd 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
bfb5eab255230b7fbfdefaa62be27a8f8c4e9517 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69720: Made `SSLTest` an unparameterized test suite.

[Benjamin Bannier]

> On Jan. 14, 2019, 7:40 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp
> > Line 417 (original), 421 (patched)
> > 
> >
> > You can consider moving this test up to group with the other 
> > parameterized tests.  (Or moving those tests down)

Some reordering here, follow-up reordering in 
https://reviews.apache.org/r/69721/.


- Benjamin


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


On Jan. 14, 2019, 8:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69720/
> ---
> 
> (Updated Jan. 14, 2019, 8:54 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-5189
> https://issues.apache.org/jira/browse/MESOS-5189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This general and reusable testing class is made unparameterized since
> deriving from a parameterized test is cumbersome (e.g., requires manual
> disambiguation of possibly private types and methods inherited from the
> respective `::testing::WithParamInterface` base class).
> 
> This patch makes `SSLTest` an unparameterized test suite which eases
> reuse. We are able to remove a number of redundant test
> parameterizations along the way.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 23d7aee963b6fb489403a94500d39e3413c7fcdd 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> bfb5eab255230b7fbfdefaa62be27a8f8c4e9517 
> 
> 
> Diff: https://reviews.apache.org/r/69720/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69721: Broke up `SSLTest.ProtocolMismatch` into smaller tests.

[Benjamin Bannier]

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

(Updated Jan. 14, 2019, 8:54 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Reordered tests.


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


Repository: mesos


Description
---

This patch breaks up the different iterations in
`SSLTest.ProtocolMismatch` into smaller tests
`SSLProtocol/SSLProtocolTest.Mismatch/N`. While this slightly
increases the time to run all combinations sequentially, it leads to
much shorter aggregated run time in parallel test execution.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
bfb5eab255230b7fbfdefaa62be27a8f8c4e9517 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69656: Fixed test `MesosContainerizerSlaveRecoveryTest.ResourceStatistics`.

[Joseph Wu]

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


Fix it, then Ship it!





src/tests/slave_recovery_tests.cpp
Lines 5152-5153 (original), 5153-5154 (patched)


Not terribly impactful for the test, but since you are capturing the 
TASK_STARTING update, you could check the status before the TASK_RUNNING one.
```
  AWAIT_READY(statusStarting);
  EXPECT_EQ(TASK_STARTING, statusStarting->state());
```


- Joseph Wu


On Jan. 2, 2019, 9:16 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69656/
> ---
> 
> (Updated Jan. 2, 2019, 9:16 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Bugs: MESOS-5048
> https://issues.apache.org/jira/browse/MESOS-5048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is flaky
> due to a race between executor shutdown (due to never getting any
> tasks) and the test querying resource statistics. If the executor
> is shutdown before the statistics query, the test will fail.
> 
> This patch fixes the test by explicitly waiting for the task to
> be delivered and task status transition to `TASK_RUNNING` before
> restarting the agent. This way, the executor will not be shutdown
> after agent restart. Hence there will be no race.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 0eb47e2bdf6a46fc21b59bb85b4b89181087ccd3 
> 
> 
> Diff: https://reviews.apache.org/r/69656/diff/1/
> 
> 
> Testing
> ---
> 
> ran `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` continuously 
> without failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69708: Fixed gRPC CMake build issue on Ubuntu 14.04.

[Joseph Wu]

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




3rdparty/grpc-1.10.0.patch
Lines 22-29 (patched)


We (Mesos) does not have a fixed requirement for SSL versions, so the same 
`find_package(OpenSSL)` could give you multiple results (usually from 0.9.8 and 
later).

Since this is a SSL 1.0.1 issue only, perhaps the logic should be:
```
find_package(OpenSSL REQUIRED)

if (OPENSSL_VERSION VERSION_EQUAL "1.0.1")
  add_definitions(-DTSI_OPENSSL_ALPN_SUPPORT=0)
endif ()
```

`_VERSION` is one of many variables defined after a successful 
`find_package`.

The `VERSION_EQUAL` comparison will work even if the "tweak" component of 
the version does not match. (i.e. if `OPENSSL_VERSION` is "1.0.1s")  This is 
because CMake only performs integer comparisons when looking at versions, but 
OpenSSL denotes tweaks via letters (and these are therefore ignored by CMake).


- Joseph Wu


On Jan. 11, 2019, 4:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69708/
> ---
> 
> (Updated Jan. 11, 2019, 4:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, BenjaminVW BenjaminVW, Gastón 
> Kleiman, Joseph Wu, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-9519
> https://issues.apache.org/jira/browse/MESOS-9519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch cherry-picks an unofficial commit to disable ALPN compilation
> if only OpenSSL 1.0.1 is on the system.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 703808d063e4bba58f647b5d48b78724003bcc4e 
>   3rdparty/grpc-1.10.0.patch 655f00387a6b308b653b23053419ec05c8b22144 
>   3rdparty/grpc.md e06843c8b6038eb9fb809241686fd611d1daedc8 
> 
> 
> Diff: https://reviews.apache.org/r/69708/diff/3/
> 
> 
> Testing
> ---
> 
> OS=ubuntu:14.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
> --enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
> support/docker-build.sh
> OS=ubuntu:16.04 BUILDTOOL=cmake COMPILER=gcc CONFIGURATION='--verbose 
> --enable-libevent --enable-ssl' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1' 
> support/docker-build.sh (to make sure that it works with OpenSSL 1.0.2. Also 
> manually inspected that ALPN symbols are linked).
> Manually tested on Debian 9 to ensure that the CMake rule can find its 
> OpenSSL 1.1.0.
> 
> NOTE: The patch itself does not depend on r/69725, but the ubuntu:16.04 test 
> above does.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Joseph Wu]

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



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.

- Joseph Wu


On Jan. 13, 2019, 7:34 p.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69735/
> ---
> 
> (Updated Jan. 13, 2019, 7:34 p.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 69721: Broke up `SSLTest.ProtocolMismatch` into smaller tests.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 11, 2019, 3:22 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69721/
> ---
> 
> (Updated Jan. 11, 2019, 3:22 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-5189
> https://issues.apache.org/jira/browse/MESOS-5189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch breaks up the different iterations in
> `SSLTest.ProtocolMismatch` into smaller tests
> `SSLProtocol/SSLProtocolTest.Mismatch/N`. While this slightly
> increases the time to run all combinations sequentially, it leads to
> much shorter aggregated run time in parallel test execution.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> bfb5eab255230b7fbfdefaa62be27a8f8c4e9517 
> 
> 
> Diff: https://reviews.apache.org/r/69721/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69720: Made `SSLTest` an unparameterized test suite.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/libprocess/src/tests/ssl_tests.cpp
Line 237 (original), 241 (patched)


This test doesn't need the parameterization.  It can use the base class.



3rdparty/libprocess/src/tests/ssl_tests.cpp
Line 266 (original), 270 (patched)


This one also doesn't need parameters.



3rdparty/libprocess/src/tests/ssl_tests.cpp
Line 417 (original), 421 (patched)


You can consider moving this test up to group with the other parameterized 
tests.  (Or moving those tests down)


- Joseph Wu


On Jan. 11, 2019, 3:22 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69720/
> ---
> 
> (Updated Jan. 11, 2019, 3:22 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-5189
> https://issues.apache.org/jira/browse/MESOS-5189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This general and reusable testing class is made unparameterized since
> deriving from a parameterized test is cumbersome (e.g., requires manual
> disambiguation of possibly private types and methods inherited from the
> respective `::testing::WithParamInterface` base class).
> 
> This patch makes `SSLTest` an unparameterized test suite which eases
> reuse. We are able to remove a number of redundant test
> parameterizations along the way.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 23d7aee963b6fb489403a94500d39e3413c7fcdd 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> bfb5eab255230b7fbfdefaa62be27a8f8c4e9517 
> 
> 
> Diff: https://reviews.apache.org/r/69720/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69726: Fixed flakiness in MasterAPITest.OperationUpdatesUponOfferGone.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69726 was successfully built and tested.

Reviews applied: `['69726']`

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

- Mesos Reviewbot Windows


On Jan. 14, 2019, 4:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69726/
> ---
> 
> (Updated Jan. 14, 2019, 4:35 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It could happen that the first offer was sent before the allocator
> had been updated with the resources from the resource provider.
> 
> To further increase reliability, stopped the clock during this test.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp c597243e2e210e83a4ab7441fbcfa3198b43d849 
> 
> 
> Diff: https://reviews.apache.org/r/69726/diff/2/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests --gtest_filter="*OperationUpdates*" --gtest_repeat=5 
> --gtest_throw_on_failure`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 69726: Fixed flakiness in MasterAPITest.OperationUpdatesUponOfferGone.

[Benno Evers]

> On Jan. 12, 2019, 8:50 p.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 4830 (original), 4830 (patched)
> > 
> >
> > Could you explain what in particular is broken here (e.g., by linking a 
> > ticket)? Resource provider managers use in memory storage for their state 
> > which should be fine here (e.g., no agent restart). The message you are 
> > quoting in the commit message is just for user information.
> > 
> > Right now it is hard to get a grasp for what needs to be done for this 
> > test to be enabled again.

Interestingly, this was actually on the wrong test! (`TaskUpdatesUponAgentGone` 
vs. `OperationUpdatesUponAgentGone`).

After some investigation it also turned out that the error is not even 
windows-specific, just for some reason far more likely to happen there (i.e. 2 
out of 2 runs, as opposed to 0 out of ca. 20 on linux), so I reverted the 
disabling and included a proper fix in this review.


- Benno


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


On Jan. 14, 2019, 4:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69726/
> ---
> 
> (Updated Jan. 14, 2019, 4:35 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It could happen that the first offer was sent before the allocator
> had been updated with the resources from the resource provider.
> 
> To further increase reliability, stopped the clock during this test.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp c597243e2e210e83a4ab7441fbcfa3198b43d849 
> 
> 
> Diff: https://reviews.apache.org/r/69726/diff/2/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests --gtest_filter="*OperationUpdates*" --gtest_repeat=5 
> --gtest_throw_on_failure`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 69726: Fixed flakiness in MasterAPITest.OperationUpdatesUponOfferGone.

[Benno Evers]

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

(Updated Jan. 14, 2019, 4:35 p.m.)


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


Changes
---

Updated review to fix the flakiness instead of disabling the test.


Summary (updated)
-

Fixed flakiness in MasterAPITest.OperationUpdatesUponOfferGone.


Repository: mesos


Description (updated)
---

It could happen that the first offer was sent before the allocator
had been updated with the resources from the resource provider.

To further increase reliability, stopped the clock during this test.


Diffs (updated)
-

  src/tests/api_tests.cpp c597243e2e210e83a4ab7441fbcfa3198b43d849 


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

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


Testing
---


Thanks,

Benno Evers

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

[Andrei Budnik]

> On Jan. 14, 2019, 1:24 p.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp_parser.cpp
> > Lines 363-365 (patched)
> > 
> >
> > So `includes` is a **required** field in a seccomp profile, at least we 
> > should have `"includes": {}` in a seccomp profile, right?

Yes, correct. See 
https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/api/types/seccomp.go#L91See
 - `Includes` doesn't have `omitempty` flag.


- Andrei


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


On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> ---
> 
> (Updated Nov. 8, 2018, 3:24 p.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 68019: Added a parser for the Docker Seccomp config format.

[Qian Zhang]

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


Fix it, then Ship it!





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


s/configuration /configuration/

Ditto for all other messages starting with `Cannot determine`.



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


Kill this empty line.

I see a couple of empty lines like this, please remove them as well.



src/linux/seccomp/seccomp_parser.cpp
Lines 216-217 (patched)


A newline between.



src/linux/seccomp/seccomp_parser.cpp
Lines 363-365 (patched)


So `includes` is a **required** field in a seccomp profile, at least we 
should have `"includes": {}` in a seccomp profile, right?



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


s/Error reading/Failed to read/

And I think we do not need the word `file` in this message just like the 
message below.


- Qian Zhang


On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> ---
> 
> (Updated Nov. 8, 2018, 11:24 p.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 bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
>   src/linux/seccomp/seccomp_parser.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp_parser.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68019/diff/13/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Andrei Budnik]

> On Jan. 14, 2019, 8:31 a.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 137-139 (patched)
> > 
> >
> > Will this affect the task run by Mesos? E.g., a task may want to run a 
> > program which has `set-user-ID` bit.

Yes, `no_new_privs` flag affects the task that wants to run a program which has 
`set-user-ID` bit.
E.g., launching a `ping -c 3 8.8.8.8` fails with seccomp. You'll see a message 
in executor logs:
```
I0114 07:19:21.887670 13264 executor.cpp:706] Forked command at 13276
ping: socket: Operation not permitted
I0114 07:19:22.055352 13263 executor.cpp:1007] Command exited with status 2 
(pid: 13276)
```

Also, see my previous comment https://reviews.apache.org/r/68018/#comment297000


- Andrei


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


On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> ---
> 
> (Updated Nov. 8, 2018, 3:24 p.m.)
> 
> 
> 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/15/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Gilbert Song]

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



Is `MasterAPITest.OperationUpdatesUponAgentGone/1` failure releted to the 
change?

- Gilbert Song


On Jan. 13, 2019, 7:34 p.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69735/
> ---
> 
> (Updated Jan. 13, 2019, 7:34 p.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 68018: Added `SeccompFilter` class.

[Qian Zhang]

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




src/linux/seccomp/seccomp.cpp
Lines 137-139 (patched)


Will this affect the task run by Mesos? E.g., a task may want to run a 
program which has `set-user-ID` bit.



src/linux/seccomp/seccomp.cpp
Lines 148 (patched)



s/ContainerSeccompProfile_Architecture_Name/ContainerSeccompProfile::Architecture_Name/



src/linux/seccomp/seccomp.cpp
Lines 224 (patched)



s/ContainerSeccompProfile_Syscall_Action_Name/ContainerSeccompProfile::Syscall::Action_Name/


- Qian Zhang


On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> ---
> 
> (Updated Nov. 8, 2018, 11:24 p.m.)
> 
> 
> 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 bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
>   src/linux/seccomp/seccomp.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68018/diff/14/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>