Re: Review Request 67244: Adding enforce_container_ports flag for network ports isolation.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67244 was successfully built and tested.

Reviews applied: `['67244']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67244

- Mesos Reviewbot Windows


On May 22, 2018, 4:02 a.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67244/
> ---
> 
> (Updated May 22, 2018, 4:02 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67244/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Review Request 67244: Adding enforce_container_ports flag for network ports isolation.

[Xudong Ni via Review Board]

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

Review request for mesos.


Repository: mesos


Description
---

To reduce deployment risk, a nonenforce mode is added for network
port isolator. When this flag is set as false(default is false),
even task uses ports not in the container resources, the container
will not raise any limitation.

Add new test for this flag and update the existing tests


Diffs (updated)
-

  docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
  src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/tests/containerizer/ports_isolator_tests.cpp 
c5b9f926047792e7f9d1f0937fa5355b1dd77965 


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


Testing
---


Thanks,

Xudong Ni

Re: Review Request 67244: Adding enforce_container_ports flag for network ports isolation.

[Xudong Ni via Review Board]

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

(Updated May 22, 2018, 4:02 a.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


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


Repository: mesos


Description
---

To reduce deployment risk, a nonenforce mode is added for network
port isolator. When this flag is set as false(default is false),
even task uses ports not in the container resources, the container
will not raise any limitation.

Add new test for this flag and update the existing tests


Diffs
-

  docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
  src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/tests/containerizer/ports_isolator_tests.cpp 
c5b9f926047792e7f9d1f0937fa5355b1dd77965 


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


Testing
---


Thanks,

Xudong Ni

Re: Review Request 67243: Windows: Changed test image to custom nanoserver.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['67207', '67208', '67243']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67243

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67243/logs/mesos-tests-stdout.log):

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (112 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1034 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (34 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (38 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (74 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (752 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (776 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (725 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (750 ms total)

[--] Global test environment tear-down
[==] 981 tests from 95 test cases ran. (449793 ms total)
[  PASSED  ] 980 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] 
DockerContainerizerHealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange

 1 FAILED TEST
  YOU HAVE 220 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67243/logs/mesos-tests-stderr.log):

```
I0522 02:14:39.096686 17932 master.cpp:10843] Updating the state of task 
f4b65691-2c1d-42d4-a3df-a990bb66d0a7 of framework 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a- (latest state: TASK_KILLED, status 
updateI0522 02:14:38.937717 11956 exec.cpp:162] Version: 1.7.0
I0522 02:14:38.962725 14832 exec.cpp:236] Executor registered on agent 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a-S0
I0522 02:14:38.966693 16596 executor.cpp:178] Received SUBSCRIBED event
I0522 02:14:38.970675 16596 executor.cpp:182] Subscribed executor on 
windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net
I0522 02:14:38.971675 16596 executor.cpp:178] Received LAUNCH event
I0522 02:14:38.975675 16596 executor.cpp:665] Starting task 
f4b65691-2c1d-42d4-a3df-a990bb66d0a7
I0522 02:14:39.053674 16596 executor.cpp:485] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0522 02:14:39.071677 16596 executor.cpp:678] Forked command at 15352
I0522 02:14:39.099684  2532 exec.cpp:445] Executor asked to shutdown
I0522 02:14:39.099684 18104 executor.cpp:178] Received SHUTDOWN event
I0522 02:14:39.099684 18104 executor.cpp:781] Shutting down
I0522 02:14:39.099684 18104 executor.cpp:894] Sending SIGTERM to process tree 
at pid 15 state: TASK_KILLED)
I0522 02:14:39.096686  8016 slave.cpp:3935] Shutting down framework 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a-
I0522 02:14:39.097692  8016 slave.cpp:6656] Shutting down executor 
'f4b65691-2c1d-42d4-a3df-a990bb66d0a7' of framework 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a- at executor(1)@192.10.1.6:62930
I0522 02:14:39.098685  8016 slave.cpp:929] Agent terminating
W0522 02:14:39.099684  8016 slave.cpp:3931] Ignoring shutdown framework 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a- because it is terminating
I0522 02:14:39.099684 17932 master.cpp:10942] Removing task 
f4b65691-2c1d-42d4-a3df-a990bb66d0a7 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 5545ea6e-0878-46b7-8f30-ce8b5036ed3a- on 
agent 5545ea6e-0878-46b7-8f30-ce8b5036ed3a-S0 at slave(448)@192.10.1.6:62909 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0522 02:14:39.103761 17932 master.cpp:1293] Agent 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a-S0 at slave(448)@192.10.1.6:62909 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net) disconnected
I0522 02:14:39.103761  7552 hierarchical.cpp:344] Removed framework 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a-
I0522 02:14:39.103761 17932 master.cpp:3303] Disconnecting agent 
5545ea6e-0878-46b7-8f30-ce8b5036ed3a-S0 at slave(448)@192.10.1.6:62909 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0522 02:14:39.103761 17932 master.cpp:3322] Deactivating agent 

Re: Review Request 67243: Windows: Changed test image to custom nanoserver.

[Akash Gupta]

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

(Updated May 22, 2018, 1:39 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


Changes
---

Updated outdated comment.


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


Repository: mesos


Description
---

The test docker images was based off of the official nanoserver
powershell image that hasn't been updated for 1803 yet. Since we don't
actually need powershell for the docker tests, the image has been
changed to a custom one based off the official nanoserver image, which
is updated for 1803.


Diffs (updated)
-

  src/tests/containerizer/docker_common.hpp 
172eea31ac6006b4b624fb04a64ed4345bd9268d 
  src/tests/containerizer/docker_tests.cpp 
e4660f7a931792f33db7978870d215d2d16fa65d 
  src/tests/health_check_tests.cpp c4661b4f005c18a7cdc69ce05576bcccb79d5dee 


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

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


Testing
---

This was tested in Windows Server 1709 & 1803. Also, tested on Linux.


Thanks,

Akash Gupta

Re: Review Request 67224: Combined and renamed `csi_*_plugin_terminations` metrics.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65665, 65666, 67224]

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 May 18, 2018, 11:46 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67224/
> ---
> 
> (Updated May 18, 2018, 11:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To avoid double counting when the operator aggregates the
> `csi_controller_plugin_terminations` and `csi_node_plugin_terminations`,
> these two are now merged into `csi_plugin/container_terminations`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 63b5d7e5f10d6ad02b5cd11b119def3b4abf4180 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67224/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 67243: Windows: Changed test image to custom nanoserver.

[Akash Gupta]

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

Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

The test docker images was based off of the official nanoserver
powershell image that hasn't been updated for 1803 yet. Since we don't
actually need powershell for the docker tests, the image has been
changed to a custom one based off the official nanoserver image, which
is updated for 1803.


Diffs
-

  src/tests/containerizer/docker_common.hpp 
172eea31ac6006b4b624fb04a64ed4345bd9268d 
  src/tests/containerizer/docker_tests.cpp 
e4660f7a931792f33db7978870d215d2d16fa65d 
  src/tests/health_check_tests.cpp c4661b4f005c18a7cdc69ce05576bcccb79d5dee 


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


Testing
---


Thanks,

Akash Gupta

Re: Review Request 67241: Added isolator logs for volume/secret isolator and container logger.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67241 was successfully built and tested.

Reviews applied: `['67241']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67241

- Mesos Reviewbot Windows


On May 21, 2018, 11:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67241/
> ---
> 
> (Updated May 21, 2018, 11:44 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, Qian Zhang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added isolator logs for volume/secret isolator and container logger.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 517d45903bc5095f2f954b95feb61c8717d395f0 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 8071e4ee808bc825b13a6291767778d6ce3c2746 
> 
> 
> Diff: https://reviews.apache.org/r/67241/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 67241: Added isolator logs for volume/secret isolator and container logger.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/volume/secret.cpp
Lines 295-296 (patched)


I'd print out how many secrets are resolved. No need to print 
`volume/secret` because it's part of the file name printed by glog.

```
XX secrets have been resolved for container YYY
```


- Jie Yu


On May 21, 2018, 11:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67241/
> ---
> 
> (Updated May 21, 2018, 11:44 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, Qian Zhang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added isolator logs for volume/secret isolator and container logger.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 517d45903bc5095f2f954b95feb61c8717d395f0 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 8071e4ee808bc825b13a6291767778d6ce3c2746 
> 
> 
> Diff: https://reviews.apache.org/r/67241/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 67241: Added isolator logs for volume/secret isolator and container logger.

[Jie Yu]

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




src/slave/containerizer/mesos/io/switchboard.cpp
Lines 310-311 (patched)


I'd print out some infomation that's relevant to this isolator for us to 
determine the following condition `requiresServer`. For example, if the 
container has tty specified.


- Jie Yu


On May 21, 2018, 11:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67241/
> ---
> 
> (Updated May 21, 2018, 11:44 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, Qian Zhang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added isolator logs for volume/secret isolator and container logger.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 517d45903bc5095f2f954b95feb61c8717d395f0 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 8071e4ee808bc825b13a6291767778d6ce3c2746 
> 
> 
> Diff: https://reviews.apache.org/r/67241/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 67241: Added isolator logs for volume/secret isolator and container logger.

[Gilbert Song]

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

Review request for mesos, Gaston Kleiman, Jie Yu, Qian Zhang, and Vinod Kone.


Repository: mesos


Description
---

Added isolator logs for volume/secret isolator and container logger.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
517d45903bc5095f2f954b95feb61c8717d395f0 
  src/slave/containerizer/mesos/isolators/volume/secret.cpp 
8071e4ee808bc825b13a6291767778d6ce3c2746 


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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 67238: Fixed a quota-related metrics bug.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67238 was successfully built and tested.

Reviews applied: `['67238']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67238

- Mesos Reviewbot Windows


On May 21, 2018, 10:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67238/
> ---
> 
> (Updated May 21, 2018, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8932
> https://issues.apache.org/jira/browse/MESOS-8932
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a quota-related metrics bug.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/metrics.cpp 
> 5194f5b3b3f507b36f02300775230157db3dd477 
>   src/tests/master_quota_tests.cpp ddb2903c0b60f2929e39d6aa23dc924aec454c69 
> 
> 
> Diff: https://reviews.apache.org/r/67238/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` passes
> 
> Also, the modified test in this patch, `MasterQuotaTest.RemoveSingleQuota`, 
> fails without the metrics fix and passes with it.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67097: Added `linux/devices` isolator whitelist support.

[Jie Yu]

> On May 21, 2018, 7:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/devices.cpp
> > Lines 170-175 (patched)
> > 
> >
> > Any reason we only do that for the directory, not the actual device 
> > file?
> 
> James Peach wrote:
> The actual device file is supposed to be accessed by the task user, so it 
> has to be owned by that user. This chown is ensuring that we don't also grant 
> access to other host users as a side-effect of the container.
> 
> Jie Yu wrote:
> What do you mean by "side-effect"?
> 
> James Peach wrote:
> We only want to grant access to the containerized task, not to other 
> processes that might be running on the host. For example, when we grant read 
> access, we set `S_IRGRP | S_IROTH` which makes the device readable by 
> everyone on the host. Restricting the permissions on the container devices 
> directory prevents this, and setting the owner ensures we will still have 
> access even if we are later running as the task user.

What I don't follow is that why we need to set the ownership and permission on 
the devicesDir. What if we just change the actual device files to have proper 
ownership (task's user) and permission (700), will that be sufficient?


- Jie


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


On May 15, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67097/
> ---
> 
> (Updated May 15, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `linux/devices` isolator support for populating the container
> devices.  This introduces a general mechanism for populating devices
> into a specific container but currently only implements devices for all
> containers based on the devices specified by the `--allowed_devices`
> agent flag.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/devices.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/devices.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67097/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

[Xudong Ni via Review Board]

> On May 21, 2018, 8:56 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 577 (patched)
> > 
> >
> > This is more complicated than it needs to be. You can simply do this:
> > 
> > ```
> > if (!enforceContainerPorts) {
> >   if (info->activePorts.isSome() &&
> >   info->activePorts == listeners) {
> > VLOG(2) << "Skipping container ports violation log";
> > continue;
> >   }
> > 
> >   // Cache the last listeners sample so that we will
> >   // only log new ports resource violations.
> >   info->activePorts = listeners; 
> > }
> > 
> > 
> > ```

Not sure I fully understand the idea, correct me if I am wrong; In the hashmap, 
the listener ports is a set of ports which may have both allocated and 
unallocated ports(the same applied to loggedPorts and unloggedPorts, we can not 
either use "==" or contains() set function to compare the set. We need set 
operation substraction to get either unallocated or unlogged set.


- Xudong


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


On May 21, 2018, 4:14 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 21, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

[Joseph Wu]

> On May 17, 2018, 11:59 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/archiver.hpp
> > Lines 93 (patched)
> > 
> >
> > Does libarchive really require a CRT file descriptor, it's not happy 
> > with a `HANDLE`?

Looks like libarchive uses Posix functions on the given FD, so it probably 
expects a CRT FD.
https://github.com/libarchive/libarchive/blob/v3.3.2/libarchive/archive_read_open_fd.c#L73


- Joseph


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


On May 14, 2018, 3:42 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated May 14, 2018, 3:42 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/3/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67097: Added `linux/devices` isolator whitelist support.

[James Peach]

> On May 21, 2018, 7:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/devices.cpp
> > Lines 170-175 (patched)
> > 
> >
> > Any reason we only do that for the directory, not the actual device 
> > file?
> 
> James Peach wrote:
> The actual device file is supposed to be accessed by the task user, so it 
> has to be owned by that user. This chown is ensuring that we don't also grant 
> access to other host users as a side-effect of the container.
> 
> Jie Yu wrote:
> What do you mean by "side-effect"?

We only want to grant access to the containerized task, not to other processes 
that might be running on the host. For example, when we grant read access, we 
set `S_IRGRP | S_IROTH` which makes the device readable by everyone on the 
host. Restricting the permissions on the container devices directory prevents 
this, and setting the owner ensures we will still have access even if we are 
later running as the task user.


- James


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


On May 15, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67097/
> ---
> 
> (Updated May 15, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `linux/devices` isolator support for populating the container
> devices.  This introduces a general mechanism for populating devices
> into a specific container but currently only implements devices for all
> containers based on the devices specified by the `--allowed_devices`
> agent flag.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/devices.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/devices.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67097/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 67238: Fixed a quota-related metrics bug.

[Benjamin Mahler]

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


Ship it!




- Benjamin Mahler


On May 21, 2018, 10:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67238/
> ---
> 
> (Updated May 21, 2018, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8932
> https://issues.apache.org/jira/browse/MESOS-8932
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a quota-related metrics bug.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/metrics.cpp 
> 5194f5b3b3f507b36f02300775230157db3dd477 
>   src/tests/master_quota_tests.cpp ddb2903c0b60f2929e39d6aa23dc924aec454c69 
> 
> 
> Diff: https://reviews.apache.org/r/67238/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` passes
> 
> Also, the modified test in this patch, `MasterQuotaTest.RemoveSingleQuota`, 
> fails without the metrics fix and passes with it.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 67238: Fixed a quota-related metrics bug.

[Greg Mann]

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

Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
Vinod Kone.


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


Repository: mesos


Description
---

Fixed a quota-related metrics bug.


Diffs
-

  src/master/allocator/mesos/metrics.cpp 
5194f5b3b3f507b36f02300775230157db3dd477 
  src/tests/master_quota_tests.cpp ddb2903c0b60f2929e39d6aa23dc924aec454c69 


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


Testing
---

`make check` passes

Also, the modified test in this patch, `MasterQuotaTest.RemoveSingleQuota`, 
fails without the metrics fix and passes with it.


Thanks,

Greg Mann

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

[Joseph Wu]

> On May 17, 2018, 11:59 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/archiver.hpp
> > Lines 68-70 (patched)
> > 
> >
> > Wait, this is C++, can't we `s/struct archive/archive`? (Thinking aloud 
> > here; should we typedef it locally so the typename is Capitalized?)

Some bits of style or naming inconsistency is unavoidable when interfacing with 
thirdparty headers.  We shouldn't over-engineer this file to match our style.


- Joseph


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


On May 14, 2018, 3:42 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated May 14, 2018, 3:42 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/3/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67097: Added `linux/devices` isolator whitelist support.

[Jie Yu]

> On May 21, 2018, 7:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/devices.cpp
> > Lines 170-175 (patched)
> > 
> >
> > Any reason we only do that for the directory, not the actual device 
> > file?
> 
> James Peach wrote:
> The actual device file is supposed to be accessed by the task user, so it 
> has to be owned by that user. This chown is ensuring that we don't also grant 
> access to other host users as a side-effect of the container.

What do you mean by "side-effect"?


- Jie


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


On May 15, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67097/
> ---
> 
> (Updated May 15, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `linux/devices` isolator support for populating the container
> devices.  This introduces a general mechanism for populating devices
> into a specific container but currently only implements devices for all
> containers based on the devices specified by the `--allowed_devices`
> agent flag.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/devices.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/devices.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67097/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 67064: Added libarchive, bzip2, and xz patches and associated build changes.

[Joseph Wu]

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




3rdparty/CMakeLists.txt
Lines 758-759 (patched)


bzip doesn't seem to have a subtitle for itself (the one here is `zlib`s).  
So you could just call it a `high-quality data compressor`.

Also the link should not have an `https`:
http://www.bzip.org/



3rdparty/CMakeLists.txt
Lines 797 (patched)


Ditto about the subtitle.



3rdparty/CMakeLists.txt
Lines 921 (patched)


This option is unconditionally added a little below.  So in the non-Windows 
case, you'd end up with an option list of: `-DENABLE_TEST=OFF ... 
-DENABLE_TEST=ON`



3rdparty/Makefile.am
Lines 280-286 (patched)


There seems to be mixed tabs/spaces here (and unfortunately, originally 
throughout this file D: ).  You can set your tab settings to 8-spaces wide to 
see if the trailing backslaces line up.

Also, why are `CPPFLAGS` and `LDFLAGS` emptied?  And should we be passing 
`$(CONFIGURE_ARGS)` to the configure script here?



3rdparty/bzip2-1.0.6.patch
Lines 7-20 (patched)


This seems very inconsistent style-wise...

Also, can you add a note saying that `bzip2.c` and `bzip2recover.c` are not 
built because we are only interested in building the library (not executables).



3rdparty/bzip2-1.0.6.patch
Lines 12-14 (patched)


You can probably move the module definitions file `libbz2.def` into the 
list of sources.



src/Makefile.am
Lines 221-226 (patched)


This looks like a mistake.  Looks like you had temporarily commented parts 
of this out?


- Joseph Wu


On May 15, 2018, 10:09 a.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67064/
> ---
> 
> (Updated May 15, 2018, 10:09 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These libraries being available will allow stout to be changed such that
> stout can invoke libarchive instead of shelling out to tar to extract
> archives. On Windows, tar usually doesn't exist, and even if it does, it
> rarely supports most compression formats. On Windows, we will build
> libarchive to support each of those compression formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 0075c133ef60bfef06f4123c14cae8a5672440f5 
>   3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 
>   3rdparty/bzip2-1.0.6.patch PRE-CREATION 
>   3rdparty/cmake/Versions.cmake 2828acd2aa00c8778c6c462bc594716b2b6289ba 
>   3rdparty/libarchive-3.3.2.patch PRE-CREATION 
>   3rdparty/versions.am ada998d62d012a45b2cf17256c1ae16b92d611f2 
>   3rdparty/xz-5.2.3.patch PRE-CREATION 
>   configure.ac 6e91ecf4659ebec2e124ca4b2218c830608abeb0 
>   src/Makefile.am 7e91681e3b8b217f8b23fa5347e059640c62fc65 
> 
> 
> Diff: https://reviews.apache.org/r/67064/diff/4/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67235: Added per-framework metrics for types of resources contained in offers.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67235 was successfully built and tested.

Reviews applied: `['66882', '66819', '66820', '66822', '66823', '66845', 
'66825', '66846', '66847', '66841', '66842', '66843', '67147', '66844', 
'66855', '66861', '66856', '66870', '66874', '67043', '66883', '67187', 
'67235']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67235

- Mesos Reviewbot Windows


On May 21, 2018, 7:03 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67235/
> ---
> 
> (Updated May 21, 2018, 7:03 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8940
> https://issues.apache.org/jira/browse/MESOS-8940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per-framework metrics for types of resources contained in offers.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
>   src/tests/master_tests.cpp 6a0fa988aebaf1cf66abaaca0c628f981c6ccc31 
> 
> 
> Diff: https://reviews.apache.org/r/67235/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67097: Added `linux/devices` isolator whitelist support.

[James Peach]

> On May 21, 2018, 7:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/devices.cpp
> > Lines 170-175 (patched)
> > 
> >
> > Any reason we only do that for the directory, not the actual device 
> > file?

The actual device file is supposed to be accessed by the task user, so it has 
to be owned by that user. This chown is ensuring that we don't also grant 
access to other host users as a side-effect of the container.


- James


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


On May 15, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67097/
> ---
> 
> (Updated May 15, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `linux/devices` isolator support for populating the container
> devices.  This introduces a general mechanism for populating devices
> into a specific container but currently only implements devices for all
> containers based on the devices specified by the `--allowed_devices`
> agent flag.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/devices.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/devices.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67097/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

[James Peach]

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



Also, please remove the review URL from the commit message. The 
`./support/post-reviews.py` script will manage this automatically for you.

- James Peach


On May 21, 2018, 4:14 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 21, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

[James Peach]

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




src/slave/containerizer/mesos/isolators/network/ports.hpp
Lines 96 (patched)


Call this
```
Option allocatedPorts;
Option activePorts;
```

Do the `/ports/allocatedPorts` in a separate patch to make is easy to 
distinguish from functional changes.



src/slave/containerizer/mesos/isolators/network/ports.hpp
Lines 102 (patched)


Call this `enforceContainerPorts`.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 577 (patched)


This is more complicated than it needs to be. You can simply do this:

```
if (!enforceContainerPorts) {
  if (info->activePorts.isSome() &&
  info->activePorts == listeners) {
VLOG(2) << "Skipping container ports violation log";
continue;
  }

  // Cache the last listeners sample so that we will
  // only log new ports resource violations.
  info->activePorts = listeners; 
}

```



src/tests/containerizer/ports_isolator_tests.cpp
Lines 941 (patched)


Double blank line before and after this function.


- James Peach


On May 21, 2018, 4:14 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 21, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

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

(Updated May 21, 2018, 1:35 p.m.)


Review request for mesos, Chun-Hung Hsiao and Joseph Wu.


Changes
---

Put back short circuit, with a note.


Repository: mesos


Description
---

This function does not need to be Unicode, and so by using the ANSI
version instead, we can avoid the `stringify` (and ``)
dependency. We also only need to include `errorbase.hpp`, not
`error.hpp`.

Added unit tests for the `WindowsError` constructor. Removed the logic
to return early when the code is 0 because, on Windows, that still
corresponds to the message "The operation completed successfully."

Also formatted and stylized correctly.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/error.hpp 
0bec474faf35705311db7b648edf9b4930ecc976 
  3rdparty/stout/tests/error_tests.cpp 5d5f6aa1df1e0fbfa466d19267d746743b9559c6 


Diff: https://reviews.apache.org/r/67236/diff/6/

Changes: https://reviews.apache.org/r/67236/diff/5-6/


Testing
---

Built and tested stout-tests on Windows.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On May 21, 2018, 1:16 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67236/
> ---
> 
> (Updated May 21, 2018, 1:16 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function does not need to be Unicode, and so by using the ANSI
> version instead, we can avoid the `stringify` (and ``)
> dependency. We also only need to include `errorbase.hpp`, not
> `error.hpp`.
> 
> Added unit tests for the `WindowsError` constructor. Removed the logic
> to return early when the code is 0 because, on Windows, that still
> corresponds to the message "The operation completed successfully."
> 
> Also formatted and stylized correctly.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/error.hpp 
> 0bec474faf35705311db7b648edf9b4930ecc976 
>   3rdparty/stout/tests/error_tests.cpp 
> 5d5f6aa1df1e0fbfa466d19267d746743b9559c6 
> 
> 
> Diff: https://reviews.apache.org/r/67236/diff/5/
> 
> 
> Testing
> ---
> 
> Built and tested stout-tests on Windows.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

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

(Updated May 21, 2018, 1:16 p.m.)


Review request for mesos, Chun-Hung Hsiao and Joseph Wu.


Changes
---

Fixed formatting of unit tests.


Repository: mesos


Description
---

This function does not need to be Unicode, and so by using the ANSI
version instead, we can avoid the `stringify` (and ``)
dependency. We also only need to include `errorbase.hpp`, not
`error.hpp`.

Added unit tests for the `WindowsError` constructor. Removed the logic
to return early when the code is 0 because, on Windows, that still
corresponds to the message "The operation completed successfully."

Also formatted and stylized correctly.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/error.hpp 
0bec474faf35705311db7b648edf9b4930ecc976 
  3rdparty/stout/tests/error_tests.cpp 5d5f6aa1df1e0fbfa466d19267d746743b9559c6 


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

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


Testing
---

Built and tested stout-tests on Windows.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

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

(Updated May 21, 2018, 1:13 p.m.)


Review request for mesos, Chun-Hung Hsiao and Joseph Wu.


Changes
---

Typo.


Repository: mesos


Description
---

This function does not need to be Unicode, and so by using the ANSI
version instead, we can avoid the `stringify` (and ``)
dependency. We also only need to include `errorbase.hpp`, not
`error.hpp`.

Added unit tests for the `WindowsError` constructor. Removed the logic
to return early when the code is 0 because, on Windows, that still
corresponds to the message "The operation completed successfully."

Also formatted and stylized correctly.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/error.hpp 
0bec474faf35705311db7b648edf9b4930ecc976 
  3rdparty/stout/tests/error_tests.cpp 5d5f6aa1df1e0fbfa466d19267d746743b9559c6 


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

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


Testing
---

Built and tested stout-tests on Windows.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

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

(Updated May 21, 2018, 1:11 p.m.)


Review request for mesos, Chun-Hung Hsiao and Joseph Wu.


Changes
---

Added tests.


Repository: mesos


Description (updated)
---

This function does not need to be Unicode, and so by using the ANSI
version instead, we can avoid the `stringify` (and ``)
dependency. We also only need to include `errorbase.hpp`, not
`error.hpp`.

Added unit tests for the `WindowsError` constructor. Removed the logic
to return early when the code is 0 because, on Windows, that still
corresponds to the message "The operation completed successfully."

Also formatted and stylized correctly.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/error.hpp 
0bec474faf35705311db7b648edf9b4930ecc976 
  3rdparty/stout/tests/error_tests.cpp 5d5f6aa1df1e0fbfa466d19267d746743b9559c6 


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

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


Testing
---

Built and tested stout-tests on Windows.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.

[Greg Mann]

> On May 18, 2018, 5:26 p.m., Greg Mann wrote:
> > src/slave/containerizer/composing.cpp
> > Line 669 (original), 618 (patched)
> > 
> >
> > Why return `wait()` when the state is DESTROYING, rather than just 
> > forwarding the call to the underlying containerizer's `destroy()`?
> > 
> > With this implementation, the composing containerizer's `wait()` will 
> > behave differently than the underlying containerizer's `wait()`, is that 
> > what we want?
> 
> Andrei Budnik wrote:
> Good point!
> It looks like `container->containerizer->destroy(containerId).onAny(...)` 
> can be moved from `if` condition to the bottom of the method. That will 
> simplify `ComposingContainerizerProcess::destroy()` implementation.
> WDYT?
> 
> Qian Zhang wrote:
> > Why return wait() when the state is DESTROYING, rather than just 
> forwarding the call to the underlying containerizer's destroy()?
> 
> `DESTROYING` state means a call to the underlying containerizer's 
> `destroy()` is already going on, so why do we want to call it again in this 
> case?
> 
> > With this implementation, the composing containerizer's wait() will 
> behave differently than the underlying containerizer's wait(), is that what 
> we want?
> 
> How will the composing containerizer's `wait()` behave differently than 
> the underlying containerizer's `wait()`? I see it is actually just a wrapper 
> of the underlying containerizer's `wait()`.
> 
> Greg Mann wrote:
> > DESTROYING state means a call to the underlying containerizer's 
> destroy() is already going on, so why do we want to call it again in this 
> case?
> 
> My logic was that the behavior of the composing containerizer should be 
> the same as the individual containerizers. So, if the caller calls 
> `destroy()` a second time, we can just forward the call to the individual 
> containerizer.
> 
> > How will the composing containerizer's wait() behave differently than 
> the underlying containerizer's wait()? I see it is actually just a wrapper of 
> the underlying containerizer's wait().
> 
> Sorry, what I meant to type was: With this implementation, the composing 
> containerizer's `destroy()` will behave differently than the underlying 
> containerizer's `destroy()`, is that what we want?

> It looks like container->containerizer->destroy(containerId).onAny(...) can 
> be moved from if condition to the bottom of the method. That will simplify 
> ComposingContainerizerProcess::destroy() implementation.

Andrei that sounds good to me - Qian what do you think?


- Greg


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


On April 17, 2018, 3:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8/
> ---
> 
> (Updated April 17, 2018, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8712
> https://issues.apache.org/jira/browse/MESOS-8712
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we stored `destroyed` promise for each container and used
> it to guarantee that `destroy()` returns a non-empty value when the
> destroy-in-progress stops an launch-in-progress using the next
> containerizer. Since `wait()` and `destroy()` return the same
> `ContainerTermination` value when called with the same ContainerID
> argument, we can remove `destroyed` promise and add callbacks to
> clean up `containers_` map instead.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> 1fb79f53b48154ecbd3b0165b6a8002c871e6dce 
> 
> 
> Diff: https://reviews.apache.org/r/8/diff/4/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.

[Greg Mann]

> On May 18, 2018, 5:26 p.m., Greg Mann wrote:
> > src/slave/containerizer/composing.cpp
> > Line 669 (original), 618 (patched)
> > 
> >
> > Why return `wait()` when the state is DESTROYING, rather than just 
> > forwarding the call to the underlying containerizer's `destroy()`?
> > 
> > With this implementation, the composing containerizer's `wait()` will 
> > behave differently than the underlying containerizer's `wait()`, is that 
> > what we want?
> 
> Andrei Budnik wrote:
> Good point!
> It looks like `container->containerizer->destroy(containerId).onAny(...)` 
> can be moved from `if` condition to the bottom of the method. That will 
> simplify `ComposingContainerizerProcess::destroy()` implementation.
> WDYT?
> 
> Qian Zhang wrote:
> > Why return wait() when the state is DESTROYING, rather than just 
> forwarding the call to the underlying containerizer's destroy()?
> 
> `DESTROYING` state means a call to the underlying containerizer's 
> `destroy()` is already going on, so why do we want to call it again in this 
> case?
> 
> > With this implementation, the composing containerizer's wait() will 
> behave differently than the underlying containerizer's wait(), is that what 
> we want?
> 
> How will the composing containerizer's `wait()` behave differently than 
> the underlying containerizer's `wait()`? I see it is actually just a wrapper 
> of the underlying containerizer's `wait()`.

> DESTROYING state means a call to the underlying containerizer's destroy() is 
> already going on, so why do we want to call it again in this case?

My logic was that the behavior of the composing containerizer should be the 
same as the individual containerizers. So, if the caller calls `destroy()` a 
second time, we can just forward the call to the individual containerizer.

> How will the composing containerizer's wait() behave differently than the 
> underlying containerizer's wait()? I see it is actually just a wrapper of the 
> underlying containerizer's wait().

Sorry, what I meant to type was: With this implementation, the composing 
containerizer's `destroy()` will behave differently than the underlying 
containerizer's `destroy()`, is that what we want?


- Greg


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


On April 17, 2018, 3:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8/
> ---
> 
> (Updated April 17, 2018, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8712
> https://issues.apache.org/jira/browse/MESOS-8712
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we stored `destroyed` promise for each container and used
> it to guarantee that `destroy()` returns a non-empty value when the
> destroy-in-progress stops an launch-in-progress using the next
> containerizer. Since `wait()` and `destroy()` return the same
> `ContainerTermination` value when called with the same ContainerID
> argument, we can remove `destroyed` promise and add callbacks to
> clean up `containers_` map instead.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> 1fb79f53b48154ecbd3b0165b6a8002c871e6dce 
> 
> 
> Diff: https://reviews.apache.org/r/8/diff/4/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

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

(Updated May 21, 2018, 12:58 p.m.)


Review request for mesos, Chun-Hung Hsiao and Joseph Wu.


Changes
---

Fixed comment.


Repository: mesos


Description
---

This function does not need to be Unicode, and so by using the ANSI
version instead, we can avoid the `stringify` (and ``)
dependency. We also only need to include `errorbase.hpp`, not
`error.hpp`. Formatted and stylized correctly.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/error.hpp 
0bec474faf35705311db7b648edf9b4930ecc976 


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

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


Testing
---

Built and tested stout-tests on Windows.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67097: Added `linux/devices` isolator whitelist support.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/linux/devices.cpp
Lines 170-175 (patched)


Any reason we only do that for the directory, not the actual device file?


- Jie Yu


On May 15, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67097/
> ---
> 
> (Updated May 15, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `linux/devices` isolator support for populating the container
> devices.  This introduces a general mechanism for populating devices
> into a specific container but currently only implements devices for all
> containers based on the devices specified by the `--allowed_devices`
> agent flag.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/devices.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/devices.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67097/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

> On May 21, 2018, 12:11 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/windows/error.hpp
> > Line 88 (original), 85 (patched)
> > 
> >
> > ```
> > so we can avoid needing Unicode conversion here.
> > ```
> > 
> > We don't need it in the `Error` class right? ;)

Yeah... I deleted the include above. Did I miss something?


- Andrew


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


On May 21, 2018, 12:02 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67236/
> ---
> 
> (Updated May 21, 2018, 12:02 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function does not need to be Unicode, and so by using the ANSI
> version instead, we can avoid the `stringify` (and ``)
> dependency. We also only need to include `errorbase.hpp`, not
> `error.hpp`. Formatted and stylized correctly.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/error.hpp 
> 0bec474faf35705311db7b648edf9b4930ecc976 
> 
> 
> Diff: https://reviews.apache.org/r/67236/diff/1/
> 
> 
> Testing
> ---
> 
> Built and tested stout-tests on Windows.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/windows/error.hpp
Line 88 (original), 85 (patched)


```
so we can avoid needing Unicode conversion here.
```

We don't need it in the `Error` class right? ;)


- Chun-Hung Hsiao


On May 21, 2018, 7:02 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67236/
> ---
> 
> (Updated May 21, 2018, 7:02 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function does not need to be Unicode, and so by using the ANSI
> version instead, we can avoid the `stringify` (and ``)
> dependency. We also only need to include `errorbase.hpp`, not
> `error.hpp`. Formatted and stylized correctly.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/error.hpp 
> 0bec474faf35705311db7b648edf9b4930ecc976 
> 
> 
> Diff: https://reviews.apache.org/r/67236/diff/1/
> 
> 
> Testing
> ---
> 
> Built and tested stout-tests on Windows.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67235: Added per-framework metrics for types of resources contained in offers.

[Greg Mann]

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

(Updated May 21, 2018, 7:03 p.m.)


Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
Vinod Kone.


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


Repository: mesos


Description
---

Added per-framework metrics for types of resources contained in offers.


Diffs (updated)
-

  src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
  src/tests/master_tests.cpp 6a0fa988aebaf1cf66abaaca0c628f981c6ccc31 


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

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


Testing
---


Thanks,

Greg Mann

Review Request 67236: Windows: Removed unnecessary dependencies from `windows/error.hpp`.

[Andrew Schwartzmeyer]

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

Review request for mesos, Chun-Hung Hsiao and Joseph Wu.


Repository: mesos


Description
---

This function does not need to be Unicode, and so by using the ANSI
version instead, we can avoid the `stringify` (and ``)
dependency. We also only need to include `errorbase.hpp`, not
`error.hpp`. Formatted and stylized correctly.


Diffs
-

  3rdparty/stout/include/stout/windows/error.hpp 
0bec474faf35705311db7b648edf9b4930ecc976 


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


Testing
---

Built and tested stout-tests on Windows.


Thanks,

Andrew Schwartzmeyer

Review Request 67235: Added per-framework metrics for types of resources contained in offers.

[Greg Mann]

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

Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
Vinod Kone.


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


Repository: mesos


Description
---

Added per-framework metrics for types of resources contained in offers.


Diffs
-

  src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
  src/tests/master_tests.cpp 6a0fa988aebaf1cf66abaaca0c628f981c6ccc31 


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


Testing
---


Thanks,

Greg Mann

Re: Review Request 67217: Ensured `SlaveRegisteredMessage`s trigger appropriate expectations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67217]

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 May 18, 2018, 6:30 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67217/
> ---
> 
> (Updated May 18, 2018, 6:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benno Evers.
> 
> 
> Bugs: MESOS-8923
> https://issues.apache.org/jira/browse/MESOS-8923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An agent may retry `SlaveRegisteredMessage` if it does not receive
> the registration confirmation on time. In this case the confirmation
> may be sent twice. In tests with multiple registering agents, this
> may result that an expectation set for one agent is satisfied by
> a retried confirmation for another agent.
> 
> This patch unifies the way how this case is handled. An expectation
> is augmented with a matcher for with the agent pid for which the
> expectation is set.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> f3fb2244bd8f10a7bc0fe3400f96dc739f3db7ac 
>   src/tests/master_tests.cpp 6a0fa988aebaf1cf66abaaca0c628f981c6ccc31 
>   src/tests/partition_tests.cpp 390b7c8d0674d6f51a06507fdb720ba2fe95b642 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 770c4822d8c8ecbacdbd0c3d92a3cd2ff3837e3d 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67217/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.13.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 67188: Windows: Implemented `os::shell` and enabled tests.

[Andrew Schwartzmeyer]

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

(Updated May 21, 2018, 11 a.m.)


Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and 
Radhika Jandhyala.


Changes
---

Added more unit tests.


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


Repository: mesos


Description
---

Once implemented, the tests had to be fixed to use Windows commands,
and a test that ensures `os::shell` doesn't hang was added. The
implementation does not reuse `os::spawn` because it must read all the
child process's piped output before waiting on it to exit, otherwise
it will deadlock.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
8da612af2888ff4d4d458ea5b16cdb08779b6f4c 
  3rdparty/stout/tests/os_tests.cpp 419879a47eb14813b8c6cac8f4a3a3bb0b9e2bed 


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

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


Testing
---

Tested on Windows with
```
powershell -NoProfile -Command (Get-NetIPAddress -AddressFamily IPv4 
-InterfaceAlias 'vEthernet (External)').IPAddress
```

Saved in `ip-command.txt` and then fed through 
`--ip_discovery_command=file://ip-command.txt`. It is otherwise very difficult 
to quote things correctly, as it goes through several layers of parsing (e.g. 
the flags loader tries to parse it as JSON if you use `{}` at all).


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65811: Add `path::normalize` to stout for normalizing path (for POSIX only now)

[Jie Yu]

> On May 18, 2018, 4:57 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 87 (patched)
> > 
> >
> > I small nits, i'd suggest the following to make the logic a bit easier 
> > to understand:
> > ```
> > if (component != "..") {
> >   components.push_back(component);
> > } else {
> >   if (isEmpty && !isAbs) {
> > components.push_back(component);
> >   } else if (!isEmpty && components.back() == "..") {
> > components.push_back(component);
> >   } else if (!isEmpty) {
> > components.pop_back();
> >   }
> > }
> > ```
> 
> Jason Lai wrote:
> I was originally using something similar to this. I think the current 
> version should be fine, as the comment should explain it. I also just 
> reference-checked [Python's implementation of 
> `os.path.normpath`](https://github.com/python/cpython/blob/2.7/Lib/posixpath.py#L346-L347),
>  it does things exactly the same way.
> 
> Jie Yu wrote:
> The part that I found not intuitive to understand is the `else if` part. 
> That was my suggestion. I didn't put an issue on this so it's up to you.

BTW, using comments to make the code readable is the worst way.


- Jie


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


On May 17, 2018, 1:06 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated May 17, 2018, 1:06 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::normalize` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::normalize` to stout for normalizing path (for POSIX only now)

[Jie Yu]

> On May 18, 2018, 4:57 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 87 (patched)
> > 
> >
> > I small nits, i'd suggest the following to make the logic a bit easier 
> > to understand:
> > ```
> > if (component != "..") {
> >   components.push_back(component);
> > } else {
> >   if (isEmpty && !isAbs) {
> > components.push_back(component);
> >   } else if (!isEmpty && components.back() == "..") {
> > components.push_back(component);
> >   } else if (!isEmpty) {
> > components.pop_back();
> >   }
> > }
> > ```
> 
> Jason Lai wrote:
> I was originally using something similar to this. I think the current 
> version should be fine, as the comment should explain it. I also just 
> reference-checked [Python's implementation of 
> `os.path.normpath`](https://github.com/python/cpython/blob/2.7/Lib/posixpath.py#L346-L347),
>  it does things exactly the same way.

The part that I found not intuitive to understand is the `else if` part. That 
was my suggestion. I didn't put an issue on this so it's up to you.


- Jie


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


On May 17, 2018, 1:06 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated May 17, 2018, 1:06 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::normalize` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 67212: Used `*_SOME` macro for checking `Try` values.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [67212, 67211, 67210]

Failed command: python support/apply-reviews.py -n -r 67211

Error:
2018-05-21 16:35:20 URL:https://reviews.apache.org/r/67211/diff/raw/ 
[3914/3914] -> "67211.patch" [1]
error: patch failed: 3rdparty/libprocess/src/tests/jwt_tests.cpp:182
error: 3rdparty/libprocess/src/tests/jwt_tests.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/22503/console

- Mesos Reviewbot


On May 18, 2018, 3:35 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67212/
> ---
> 
> (Updated May 18, 2018, 3:35 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `*_SOME` macro for checking `Try` values.
> 
> 
> Diffs
> -
> 
>   src/tests/secret_generator_tests.cpp 
> 7fd649b2bb403d943955b5df1299c5c58b2c7582 
> 
> 
> Diff: https://reviews.apache.org/r/67212/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

[Xudong Ni via Review Board]

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

(Updated May 21, 2018, 4:14 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


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


Repository: mesos


Description
---

To reduce deployment risk, a nonenforce mode is added for network
port isolator. When this flag is set as false(default is false),
even task uses ports not in the container resources, the container
will not raise any limitation.

Add new test for this flag and update the existing tests

https://reviews.apache.org/r/67195/


Diffs
-

  docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
  src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/tests/containerizer/ports_isolator_tests.cpp 
c5b9f926047792e7f9d1f0937fa5355b1dd77965 


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


Testing
---

New test added for the flag; Related unit tests passed.

[ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)


Thanks,

Xudong Ni

Re: Review Request 66871: Added documentation about new gauge on `master/subscribers_active`.

[James Peach]

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


Fix it, then Ship it!





docs/monitoring.md
Lines 845 (patched)


`... information about the HTTP API ...`



docs/monitoring.md
Lines 855 (patched)


`... to the event stream API`


- James Peach


On April 30, 2018, 8:59 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66871/
> ---
> 
> (Updated April 30, 2018, 8:59 p.m.)
> 
> 
> Review request for mesos, Jason Lai and James Peach.
> 
> 
> Bugs: MESOS-6499
> https://issues.apache.org/jira/browse/MESOS-6499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation about new gauge on `master/subscribers_active`.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md c20b99eb32a50830ea6f66b5bdbfcabe45fc6ecb 
> 
> 
> Diff: https://reviews.apache.org/r/66871/diff/1/
> 
> 
> Testing
> ---
> 
> https://github.com/zhitaoli/mesos/blob/zhitao/public/active_subscribers/docs/monitoring.md#http-api
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67215: Opened subprocess pipes with `O_CLOEXEC`.

[James Peach]

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




3rdparty/libprocess/src/subprocess_posix.cpp
Line 50 (original), 50 (patched)


AFAICT `pipe2(2)` is available on Linux and FreeBSD but not macOS.


- James Peach


On May 18, 2018, 12:36 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67215/
> ---
> 
> (Updated May 18, 2018, 12:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Opened subprocess pipes with `O_CLOEXEC`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> d7a725363251f9c54072cd7551f5598696938308 
>   3rdparty/libprocess/src/subprocess_posix.cpp 
> 01e3272fccda6ff66e1629fb10d22d8c4967b22a 
> 
> 
> Diff: https://reviews.apache.org/r/67215/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66670: Ensured that `wait()` and `destroy()` return the same result.

[Qian Zhang]

> On May 10, 2018, 8:52 p.m., Qian Zhang wrote:
> > src/slave/containerizer/composing.cpp
> > Line 621 (original), 629-632 (patched)
> > 
> >
> > Similarly, I am wondering if we can reach here for a nested container. 
> > If a nested container terminates but has not been removed from 
> > `containers_`, then it should be still in the `LAUNCHED` state, but we can 
> > only reach here for the containers in the `DESTROYING` state.
> 
> Andrei Budnik wrote:
> Same as above.

I think here the key is the container is in the `DESTROYING` state (i.e., call 
`destroy` when a previous `destroy` is still going on) no matter it is nested 
container or not and terminated or not (it can be destroying but not terminated 
yet). So I think in this comment we should only mention case 1) but not case 2) 
which may cause confusion.


- Qian


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


On April 17, 2018, 11:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66670/
> ---
> 
> (Updated April 17, 2018, 11:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8713
> https://issues.apache.org/jira/browse/MESOS-8713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need to return the same `ContainerTermination` result for both
> `wait()` and `destroy()` for a terminated container. This patch
> ensures that for a terminated nested container `destroy()` returns
> the same result as for `wait()`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> 1fb79f53b48154ecbd3b0165b6a8002c871e6dce 
>   src/slave/containerizer/mesos/containerizer.cpp 
> eac1d16f2388385fec04ff8f013ce0ebf4e97f0f 
> 
> 
> Diff: https://reviews.apache.org/r/66670/diff/4/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.

[Qian Zhang]

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



Can you please explain how the container will be cleaned up from the 
`containers_` map after agent recovery?


src/slave/containerizer/composing.cpp
Line 361 (original), 360-365 (patched)


So besides removing the container from the `containers_` map, we also 
eliminate a call to `ComposingContainerizerProcess::destroy()`, My 
understanding is we assume the underlying containerizers will always do its own 
destroy to clean up its internal data, so here in composing containerizer we 
just need to remove the container from the `containers_` map, right?



src/slave/containerizer/composing.cpp
Lines 396-400 (original)


Previously, in the case that `destroy-in-progress stopped an 
launch-in-progress`, the `destroy()` will return `ContainerTermination()` which 
is set here. But now with this implementation, it will return `None()` which is 
set by the underlying containerizer. This seems not correct to me, in this 
case, the destroy should considered as succeeded (return 
`ContainerTermination()`) rather than failed (return `None()`).



src/slave/containerizer/composing.cpp
Lines 401-402 (original), 395-396 (patched)


Do we need these two lines? Since there is a destroy going on, the 
container will be removed from `containers_` when the destroy is done.


- Qian Zhang


On April 17, 2018, 11:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8/
> ---
> 
> (Updated April 17, 2018, 11:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8712
> https://issues.apache.org/jira/browse/MESOS-8712
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we stored `destroyed` promise for each container and used
> it to guarantee that `destroy()` returns a non-empty value when the
> destroy-in-progress stops an launch-in-progress using the next
> containerizer. Since `wait()` and `destroy()` return the same
> `ContainerTermination` value when called with the same ContainerID
> argument, we can remove `destroyed` promise and add callbacks to
> clean up `containers_` map instead.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> 1fb79f53b48154ecbd3b0165b6a8002c871e6dce 
> 
> 
> Diff: https://reviews.apache.org/r/8/diff/4/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.

[Qian Zhang]

> On May 19, 2018, 1:26 a.m., Greg Mann wrote:
> > src/slave/containerizer/composing.cpp
> > Line 669 (original), 618 (patched)
> > 
> >
> > Why return `wait()` when the state is DESTROYING, rather than just 
> > forwarding the call to the underlying containerizer's `destroy()`?
> > 
> > With this implementation, the composing containerizer's `wait()` will 
> > behave differently than the underlying containerizer's `wait()`, is that 
> > what we want?
> 
> Andrei Budnik wrote:
> Good point!
> It looks like `container->containerizer->destroy(containerId).onAny(...)` 
> can be moved from `if` condition to the bottom of the method. That will 
> simplify `ComposingContainerizerProcess::destroy()` implementation.
> WDYT?

> Why return wait() when the state is DESTROYING, rather than just forwarding 
> the call to the underlying containerizer's destroy()?

`DESTROYING` state means a call to the underlying containerizer's `destroy()` 
is already going on, so why do we want to call it again in this case?

> With this implementation, the composing containerizer's wait() will behave 
> differently than the underlying containerizer's wait(), is that what we want?

How will the composing containerizer's `wait()` behave differently than the 
underlying containerizer's `wait()`? I see it is actually just a wrapper of the 
underlying containerizer's `wait()`.


- Qian


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


On April 17, 2018, 11:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8/
> ---
> 
> (Updated April 17, 2018, 11:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8712
> https://issues.apache.org/jira/browse/MESOS-8712
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we stored `destroyed` promise for each container and used
> it to guarantee that `destroy()` returns a non-empty value when the
> destroy-in-progress stops an launch-in-progress using the next
> containerizer. Since `wait()` and `destroy()` return the same
> `ContainerTermination` value when called with the same ContainerID
> argument, we can remove `destroyed` promise and add callbacks to
> clean up `containers_` map instead.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> 1fb79f53b48154ecbd3b0165b6a8002c871e6dce 
> 
> 
> Diff: https://reviews.apache.org/r/8/diff/4/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 67136: Added a function to get rlimits.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67136]

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 May 18, 2018, 2:36 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67136/
> ---
> 
> (Updated May 18, 2018, 2:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a function to get rlimits.
> 
> 
> Diffs
> -
> 
>   src/posix/rlimits.hpp 0cc4d8aa58e400c0509969acdb5bd30bf89146da 
>   src/posix/rlimits.cpp 6446c1418349a0c3cb12d66db7d607e4cf06662d 
> 
> 
> Diff: https://reviews.apache.org/r/67136/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67177: Sorted container mounts by their target paths.

[Eric Chung]

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


Ship it!




Ship It!

- Eric Chung


On May 17, 2018, 1:28 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67177/
> ---
> 
> (Updated May 17, 2018, 1:28 a.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, James Peach, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6798
> https://issues.apache.org/jira/browse/MESOS-6798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Container mounts are sorted by:
> 1) their target paths, and then
> 2) their source paths,
> before mounting them upon container launch.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> f25d90651ef32495c9161c3eaed8a327d1b2b926 
> 
> 
> Diff: https://reviews.apache.org/r/67177/diff/1/
> 
> 
> Testing
> ---
> 
> Manual
> 
> 
> Thanks,
> 
> Jason Lai
> 
>