Re: Review Request 60492: Added a `network/ports` isolator skeleton.

[Jie Yu]

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


Fix it, then Ship it!





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


I'd prefer the order in the header to be the same in the corresponding 
source file. Please make it consistent.

We usually put 'recover' method before 'prepare' method.



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


Can we use `uint16_t`?


- Jie Yu


On Aug. 14, 2017, 11:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60492/
> ---
> 
> (Updated Aug. 14, 2017, 11:34 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the skeleton of the `network/ports` isolator and wired it up
> to the autotools build. Added the --enable-network-ports-isolator
> configuration option to build the isolator and check for the libnl3
> dependencies.
> 
> 
> Diffs
> -
> 
>   configure.ac ee3818d404013e172bc51f11e8c5792cb335a22c 
>   src/Makefile.am 68fff148f3d0c1710305bd9afcba62336d194b55 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60492/diff/9/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac ee3818d404013e172bc51f11e8c5792cb335a22c 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/5/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61538: Used common port range interval code in the port_mapping isolator.

[Jie Yu]

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



Can you explain a bit more why this change?

- Jie Yu


On Aug. 9, 2017, 8:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61538/
> ---
> 
> (Updated Aug. 9, 2017, 8:18 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched the port_mapping isolator over to start using the
> common values code to parse ranges into an IntervalSet. Since
> that is generic code, we now deal with ports as uint64_t rather
> than uint16_t.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 6ff3820de6af544c6ec92b0e76fd934715b87a96 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 57d4ccd515b538eece94a228916ec764c83473be 
> 
> 
> Diff: https://reviews.apache.org/r/61538/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60836: Added IntervalSet to Ranges conversion helper declarations.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 13, 2017, 4:53 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60836/
> ---
> 
> (Updated July 13, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new `common/values.hpp` header file to expose IntervalSet
> to Ranges conversion helper declarations.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 68fff148f3d0c1710305bd9afcba62336d194b55 
>   src/common/values.hpp PRE-CREATION 
>   src/common/values.cpp 1ac327f787b0d0ab7c704d747c0294b022e59625 
>   src/tests/values_tests.cpp 1ce9c485fa2c3ec98b5f81972472c73c82635a2b 
> 
> 
> Diff: https://reviews.apache.org/r/60836/diff/6/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60901: Use a consistent preprocessor check for ENABLE_PORT_MAPPING_ISOLATOR.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 16, 2017, 11:42 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60901/
> ---
> 
> (Updated July 16, 2017, 11:42 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There's no need to also check for Windows when testing the
> ENABLE_PORT_MAPPING_ISOLATOR feature macro, because
> ENABLE_PORT_MAPPING_ISOLATOR requires libnl3, which is a
> Linux-specific features.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ff192bb085f3554ad1b1f20fb73bf827bf04ef13 
> 
> 
> Diff: https://reviews.apache.org/r/60901/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Frdora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60764: Refactored isolator dependency checking.

[Jie Yu]

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


Fix it, then Ship it!




Thanks for the cleanup!


src/slave/containerizer/mesos/containerizer.cpp
Lines 148 (patched)


Let's just inline this method. There is only one user for this method.



src/slave/containerizer/mesos/containerizer.cpp
Line 154 (original), 169 (patched)


Can you create a LinkedHashset (similar to LinkedHashmap) in stout which 
preserve the insertion order. this solves one issues about the ordering of the 
isolators is not preserved.

I would at least add a TODO here (i do wish you can follow up :))



src/slave/containerizer/mesos/containerizer.cpp
Lines 182-184 (original), 203-208 (patched)


The indentation should be like the following:
```
switch (std::count_if(
isolations->begin(),
isolations->end(),
[](...) {
})) {
}
```

similar to what we did for other statements:
```
int foo = func(
param1,
param2);
```


- Jie Yu


On July 11, 2017, 11:13 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60764/
> ---
> 
> (Updated July 11, 2017, 11:13 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored the isolator dependency checks to immediately tokenize
> the isolator string, which makes it easier to check various consistency
> conditions.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ff192bb085f3554ad1b1f20fb73bf827bf04ef13 
> 
> 
> Diff: https://reviews.apache.org/r/60764/diff/7/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60494: Exposed LinuxLauncher cgroups helper.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 14, 2017, 11:30 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60494/
> ---
> 
> (Updated Aug. 14, 2017, 11:30 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose the LinuxLauncher cgroups helper to generate the cgroups
> path from a container ID. This is needed by the `network/ports`
> isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> e1525231e0a374bc044e929e82b8d051732e97cb 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1cea04edac8e0c4aea8c1c7d946b5065f3eac931 
> 
> 
> Diff: https://reviews.apache.org/r/60494/diff/9/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60493: Removed diagnostic socket IPv4 assumptions.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On June 28, 2017, 7:45 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60493/
> ---
> 
> (Updated June 28, 2017, 7:45 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Don't assume the diagnostic socket only returns IPv4 addresses.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/diagnosis/diagnosis.cpp 
> aa2d020fc009040410ef557d10d386d04d71d16e 
> 
> 
> Diff: https://reviews.apache.org/r/60493/diff/9/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60491: Captured the inode when scanning for sockets.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On June 28, 2017, 7:44 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60491/
> ---
> 
> (Updated June 28, 2017, 7:44 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the socket inode in the diagnosis Info when we use netlink
> to enumerate the open sockets. This can be used to identify which
> process(es) have the socket open.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/diagnosis/diagnosis.hpp 
> 55b4fc2cbf972c8c687e8eba9cc4782db891eb79 
>   src/linux/routing/diagnosis/diagnosis.cpp 
> aa2d020fc009040410ef557d10d386d04d71d16e 
> 
> 
> Diff: https://reviews.apache.org/r/60491/diff/10/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61808: Add task env overwriting executor env glog print

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61808]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 21, 2017, 6:56 p.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61808/
> ---
> 
> (Updated Aug. 21, 2017, 6:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When user assign environment by `CommandInfo` environment, it will overwrite 
> the same name environment get by before.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp b84c392 
> 
> 
> Diff: https://reviews.apache.org/r/61808/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 61189: Added authorization for V1 events.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [61262, 61189]

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

- Mesos Reviewbot


On Aug. 18, 2017, 8:33 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> ---
> 
> (Updated Aug. 18, 2017, 8:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8 
>   src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604 
>   src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 
>   src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> GLOG_v=2 ./bin/mesos-tests.sh 
> --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" 
> --verbose --gtest_repeat=100 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Review Request 61808: Add task env overwriting executor env glog print

[Andy Pang]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

When user assign environment by `CommandInfo` environment, it will overwrite 
the same name environment get by before.


Diffs
-

  src/slave/containerizer/docker.cpp b84c392 


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


Testing
---

make check


Thanks,

Andy Pang

Re: Review Request 61806: Updated depiction of runtime directory file structure.

[Joseph Wu]

> On Aug. 21, 2017, 4:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/paths.hpp
> > Lines 54 (patched)
> > 
> >
> > Align this field?

Ah whoops.

Was:
```
//   |   |-- 
//   |   |-- pid
```

Should be:
```
//   |   |   |-- 
//   |   |-- pid
```


- Joseph


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


On Aug. 21, 2017, 3:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61806/
> ---
> 
> (Updated Aug. 21, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This attempts to match the structure of the top level comments
> in "slave/paths.hpp" and "slave/containerizer/mesos/paths.hpp".
> 
> The runtime directory tree/drawing now shows all potential entries
> that can exist in the file structure and the potential nesting of
> containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.hpp 
> 12ae7c7329f9e8d334cb32c30cc38465bddc046c 
> 
> 
> Diff: https://reviews.apache.org/r/61806/diff/1/
> 
> 
> Testing
> ---
> 
> Comment change only.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 61806: Updated depiction of runtime directory file structure.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/paths.hpp
Lines 54 (patched)


Align this field?


- Jie Yu


On Aug. 21, 2017, 10:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61806/
> ---
> 
> (Updated Aug. 21, 2017, 10:21 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This attempts to match the structure of the top level comments
> in "slave/paths.hpp" and "slave/containerizer/mesos/paths.hpp".
> 
> The runtime directory tree/drawing now shows all potential entries
> that can exist in the file structure and the potential nesting of
> containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.hpp 
> 12ae7c7329f9e8d334cb32c30cc38465bddc046c 
> 
> 
> Diff: https://reviews.apache.org/r/61806/diff/1/
> 
> 
> Testing
> ---
> 
> Comment change only.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 61124: Push metric history entries with their timestamp.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On July 25, 2017, 4:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> ---
> 
> (Updated July 25, 2017, 4:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metric.hpp 
> 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721 
>   3rdparty/libprocess/include/process/metrics/timer.hpp 
> 0a9c0227c457c6c81a59f65f901a5464ee00983d 
> 
> 
> Diff: https://reviews.apache.org/r/61124/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61805: Renamed CONTAINERS_DIR to EXECUTOR_RUNS_DIR.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 21, 2017, 10:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61805/
> ---
> 
> (Updated Aug. 21, 2017, 10:21 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `CONTAINERS_DIR` will be used more accurately as the variable
> for "containers", rather than "runs".
> 
> 
> Diffs
> -
> 
>   src/slave/paths.cpp eb6a28e3bdfb520a08f6985d45f6fc1a2346d99e 
> 
> 
> Diff: https://reviews.apache.org/r/61805/diff/1/
> 
> 
> Testing
> ---
> 
> Just a rename.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 61766: Made the output handling of TCP and HTTP checks consistent.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> ---
> 
> (Updated Aug. 19, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the output handling of TCP and HTTP checks consistent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61766/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61766: Made the output handling of TCP and HTTP checks consistent.

[Greg Mann]

> On Aug. 21, 2017, 1:46 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 921-924 (original), 921-924 (patched)
> > 
> >
> > Additionally `VLOG(1)` stderr?

This failure message will be printed by the executor and it includes the stderr 
output, so I think this is sufficient?


- Greg


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


On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> ---
> 
> (Updated Aug. 19, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the output handling of TCP and HTTP checks consistent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61766/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61766: Made the output handling of TCP and HTTP checks consistent.

[Greg Mann]

> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 916-919 (original), 916-919 (patched)
> > 
> >
> > Looking at this, I'm not sure we should return a failure here: `stderr` 
> > is not critical for getting the HTTP code, so maybe log warning here? This 
> > is also consistent with what you now do for nested command checks.

In the nested command check case, the logging is performed in a void function, 
so returning a failure is not an option. I think that returning a failure here 
makes sense; while stderr isn't strictly necessary to get the status code, if 
we hit this case something has definitely gone wrong. The failure message will 
be logged by the executor and will be easily accessible for debugging.


- Greg


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


On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> ---
> 
> (Updated Aug. 19, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the output handling of TCP and HTTP checks consistent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61766/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Greg Mann]

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


Ship it!





src/checks/checker_process.cpp
Lines 648-649 (patched)


Fits on one line. I'll fix this while committing.



src/checks/checker_process.cpp
Lines 652-653 (patched)


Fits on one line. I'll fix this while committing.


- Greg Mann


On Aug. 21, 2017, 7:08 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/4/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 60888: WIP: Added recovery logic for standalone containers.

[Joseph Wu]

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

(Updated Aug. 21, 2017, 3:21 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Rework the structure completely.  Checkpointing of standalone containers is now 
handled entirely within the --runtime_dir.


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


Repository: mesos


Description (updated)
---

Although there is no way to launch standalone containers yet,
this commit outlines the expected layout of container metadata
which should be populated when launching standalone containers.

The layout is fairly simple, as standalone containers have no
framework, executor, or tasks to worry about.  The sandbox directory
will live under a new top-level directory `containers` and there is
no metadata to checkpoint at the moment.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
5772421c3078d36225b946a5286b8c1bf2f007e8 
  src/slave/containerizer/mesos/paths.hpp 
12ae7c7329f9e8d334cb32c30cc38465bddc046c 
  src/slave/containerizer/mesos/paths.cpp 
0c61c20c345a327ec469b382558aaeed0280e754 
  src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
  src/slave/paths.cpp eb6a28e3bdfb520a08f6985d45f6fc1a2346d99e 


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

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


Testing
---

See next patch and later ones too.


Thanks,

Joseph Wu

Review Request 61805: Renamed CONTAINERS_DIR to EXECUTOR_RUNS_DIR.

[Joseph Wu]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

`CONTAINERS_DIR` will be used more accurately as the variable
for "containers", rather than "runs".


Diffs
-

  src/slave/paths.cpp eb6a28e3bdfb520a08f6985d45f6fc1a2346d99e 


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


Testing
---

Just a rename.


Thanks,

Joseph Wu

Review Request 61806: Updated depiction of runtime directory file structure.

[Joseph Wu]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

This attempts to match the structure of the top level comments
in "slave/paths.hpp" and "slave/containerizer/mesos/paths.hpp".

The runtime directory tree/drawing now shows all potential entries
that can exist in the file structure and the potential nesting of
containers.


Diffs
-

  src/slave/containerizer/mesos/paths.hpp 
12ae7c7329f9e8d334cb32c30cc38465bddc046c 


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


Testing
---

Comment change only.


Thanks,

Joseph Wu

Re: Review Request 60766: Ignored containers that join CNI networks.

[James Peach]

> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 244 (original), 245 (patched)
> > 
> >
> > So here we only count `network/cni` isolator and `network/port_mapping` 
> > isolator, either of them (but not both of them) can work with 
> > `network/ports` isolator. Can you please also update the comments 
> > accordingly?

This is already commented just above.


> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 308-311 (original), 313-323 (patched)
> > 
> >
> > I think we only need to do this check for top-level container, but not 
> > for nested container since nested container always share network namespace 
> > with its parent. So we may need to add `!containerId.has_parent()` into the 
> > condition of the first `if`.

The check for nested containers needs to be separate since the child might be 
nested within a CNI network or a host network. When `prepare` a nested 
container, we only isolate it if we already isolated the corresponding root of 
the container tree.


> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 392-402 (patched)
> > 
> >
> > Can we check `state->executor_info().container().network_infos()` 
> > rather than checking CNI container dir?

Are we guaranteed to have a named network in `state->executor_info` in the case 
of nested containers joining the parent network? If not, then I think we still 
have to check whether the root container has a CNI configuration. I updated the 
patch to do this.


- James


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


On Aug. 21, 2017, 10:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> ---
> 
> (Updated Aug. 21, 2017, 10:01 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 5772421c3078d36225b946a5286b8c1bf2f007e8 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60766/diff/9/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60766: Ignored containers that join CNI networks.

[James Peach]

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

(Updated Aug. 21, 2017, 10:01 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


Changes
---

Addressed review feedback and updated to handle nested CNI containers.


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


Repository: mesos


Description
---

Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
5772421c3078d36225b946a5286b8c1bf2f007e8 
  src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/60766/diff/9/

Changes: https://reviews.apache.org/r/60766/diff/8-9/


Testing
---

make check (Fedora 26).


Thanks,

James Peach

Re: Review Request 61791: Raised the logging level of some (health) check messages.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Aug. 21, 2017, 7:46 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61791/
> ---
> 
> (Updated Aug. 21, 2017, 7:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some users pointed out that always logging the result of (health)
> checks makes debugging problems easier.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61791/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61801: Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61797, 61798, 61799, 61800, 61801]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 21, 2017, 4:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61801/
> ---
> 
> (Updated Aug. 21, 2017, 4:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> c5d83e667da0b2c8c03937c66e2a973d547cd3d2 
> 
> 
> Diff: https://reviews.apache.org/r/61801/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61664: Libprocess: Added a timeout for send socket operation.

[Benjamin Mahler]

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



Thanks Alex! A couple of things of note:

(1) Looks like the libevent ssl socket does not support send discards? Can you 
implement that?
(2) Can we safely add a default here given the master's health checking and 
http heartbeating? If users set this flag, does it conflict with the master's 
timeouts?

See below for details!


3rdparty/libprocess/src/process.cpp
Lines 251 (patched)


Maybe http_socket_send_timeout so that we can namespace all http related 
configuration together?



3rdparty/libprocess/src/process.cpp
Lines 252-255 (patched)


Hm.. how about clarifying that this relates to the clients and that the 
connection will be closed?

```
Timeout between successive writes (send / sendfile) when transmitting a 
response to a client. If the client does not make progress within this time, 
the connection is closed.
```



3rdparty/libprocess/src/process.cpp
Lines 256-261 (patched)


I don't think we can choose a default for this without conflicting with the 
master's health checking of agents (and http schedulers?), no?

On that note, what impact does setting this flag have on existing 
functionality that has related timeouts (i.e. the master's health checking, and 
http scheduler heartbeats?)



3rdparty/libprocess/src/process.cpp
Lines 2183-2188 (patched)


I did an audit of the poll_socket.cpp path and it handles discards 
correctly FWICT, however the libevent_ssl_socket.cpp path does not :(

Before this patch, can you implement discard in the libevent ssl socket 
code?

As an aside, I'm still hoping to remove libevent from the project and have 
a single event loop choice since libev is a better designed library for our 
needs (it looks like nghttp2 may make this an easier task). :)



3rdparty/libprocess/src/process.cpp
Lines 2187 (patched)


Hm.. this should be returning the future itself:

```
  auto discard = [](Future future) {
future.discard();
return future;
  };
```

That way, you don't mask the underlying result. It's possible we try to 
discard but it's already returned some length, in which case we should just 
continue sending. Or, it's possible that some other failure occurred and we 
mask it. Also, if the discard is buggy and hangs, I would not want to mask that 
issue by proceeding immediately here.

Then, in `_send`, we handle the DISCARDED case as triggering the timeout. 
To clean it up a bit, you could then use `.recover` that benh is introducing to 
convert DISCARDED to the failure message you want (but for now it seems ok to 
have the timeout message within `_send`).


- Benjamin Mahler


On Aug. 15, 2017, 3:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61664/
> ---
> 
> (Updated Aug. 15, 2017, 3:45 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7748
> https://issues.apache.org/jira/browse/MESOS-7748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, a send socket operation can wait forever for
> a send to complete. Clients that drop connections or stop reading
> incoming data, aka "slow reader" attack, can eventually exhaust the
> resources of a libprocess-based application and cause denial of
> service or an OOM event.
> 
> This patch introduces an obligatory timeout for all send socket
> operations, after which the stalled connection is dropped. The
> timeout can be adjusted via the `LIBPROCESS_SOCKET_SEND_TIMEOUT`
> environment variable.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> dcd9c6738816764aae066fe56cd5f468c98fc9bd 
> 
> 
> Diff: https://reviews.apache.org/r/61664/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing with a rogue client.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61588: Added a `[-s|--skip-hooks]` option when applying reviews.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [61588]

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

- Mesos Reviewbot


On Aug. 11, 2017, 2:03 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61588/
> ---
> 
> (Updated Aug. 11, 2017, 2:03 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7880
> https://issues.apache.org/jira/browse/MESOS-7880
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a `[-s|--skip-hooks]` option when applying reviews.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py aa2eb8d23bed52a4f33510c53ace2c63bd66ec49 
> 
> 
> Diff: https://reviews.apache.org/r/61588/diff/3/
> 
> 
> Testing
> ---
> 
> Applied a review chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 61261: Documented agent garbage collection metrics.

[Jiang Yan Xu]

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




docs/monitoring.md
Lines 1338 (patched)


s/garbage collection process// ?

Too much of internals for a user doc?

Here and below.



docs/monitoring.md
Lines 1345 (patched)


s/scheduled for/pending/ just to avoid confusion?


- Jiang Yan Xu


On July 31, 2017, 9:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61261/
> ---
> 
> (Updated July 31, 2017, 9:31 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7842
> https://issues.apache.org/jira/browse/MESOS-7842
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented the following agent sandbox garbage collection metrics:
>   - slave/gc_path_removals_failed
>   - slave/gc_path_removals_pending
>   - slave/gc_path_removals_succeeded
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md d1f64d46b22e79c16a680a2e7fff9a01202c5457 
> 
> 
> Diff: https://reviews.apache.org/r/61261/diff/2/
> 
> 
> Testing
> ---
> 
> None.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61260: Added agent garbage collection metrics.

[Jiang Yan Xu]

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



Can we cover the failure case in the tests? e.g., 
`GarbageCollectorIntegrationTest.ROOT_BusyMountPoint`?


src/slave/gc.cpp
Lines 72 (patched)


Can we add a simple comment on why `await()` is used? e.g.,

```
// Make sure the gauge is not accessed after the destruction of `gc`.
```



src/slave/gc.cpp
Lines 196-198 (patched)


IIRC from your slack chat with @mpark this (unable to use `mutable`) is a 
regression and it should be fixed soon? If so can we make sure a JIRA is filed 
and a TODO that references it is added here?


- Jiang Yan Xu


On Aug. 10, 2017, 3 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61260/
> ---
> 
> (Updated Aug. 10, 2017, 3 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7842
> https://issues.apache.org/jira/browse/MESOS-7842
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some basic sandbox garbage collection metrics to track the number
> of paths removals, the number of failed path removal and the number of
> paths that are still scheduled for removal.
> 
> 
> Diffs
> -
> 
>   src/slave/gc.cpp f270fa47b22904e926f4bd4b0a7693a41b2c8271 
>   src/slave/gc_process.hpp PRE-CREATION 
>   src/tests/gc_tests.cpp d4daad3993bb9a92db2c7add6e74f12aeb71d535 
> 
> 
> Diff: https://reviews.apache.org/r/61260/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 61801: Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and James Peach.


Repository: mesos


Description
---

Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.


Diffs
-

  src/tests/containerizer/cgroups_tests.cpp 
c5d83e667da0b2c8c03937c66e2a973d547cd3d2 


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


Testing
---

sudo make check (mac os x, fedora 25)
internal CI


Thanks,

Andrei Budnik

Review Request 61800: Enhanced async-signal safety of `signalSafeWriteStatus` function.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and James Peach.


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


Repository: mesos


Description
---

Enhanced async-signal safety of `signalSafeWriteStatus` function.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
0affcf596283f7ae2ed1899afbaa3c7d2b23f425 


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


Testing
---

sudo make check (mac os x, fedora 25)
internal CI


Thanks,

Andrei Budnik

Review Request 61799: Replaced ABORT with _EXIT in `childMain` to handle `os::execvpe` error.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and James Peach.


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


Repository: mesos


Description
---

Using ABORT causes coredump when path to an executable is invalid.
In addition, using of _EXIT allows us to get rid of string
concatenation, thereby making `childMain` more async-signal safe.


Diffs
-

  3rdparty/libprocess/src/subprocess_posix.hpp 
b478beccff22e2ffa08b9b91cfd5b9c6ada9b697 


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


Testing
---

sudo make check (mac os x, fedora 25)
internal CI


Thanks,

Andrei Budnik

Review Request 61797: Added async-signal safe implementation of `write` function in stout.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and James Peach.


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


Repository: mesos


Description
---

Async-signal safe `write` function, which can take variable number of
arguments, should be used to improve signal safety.


Diffs
-

  3rdparty/stout/include/stout/os/write.hpp 
beb5bd83b52565a75e34d32b5bb17951bc799578 


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


Testing
---

sudo make check (mac os x, fedora 25)
internal CI


Thanks,

Andrei Budnik

Review Request 61798: Added _EXIT as alternative to ABORT.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and James Peach.


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


Repository: mesos


Description
---

EXIT macro can't be used when async-signal safety is required.

Currently, ABORT is used for terminating a program after both
unexpected and expected errors, when async-signal safety is required.
In case of expected errors, `abort` causes coredumps, which might be
undesirable.

In addition, _EXIT takes variable number of arguments, thereby _EXIT
interface doesn't force users to concatenate strings, thus making its
usage more async-signal safe.


Diffs
-

  3rdparty/stout/include/stout/exit.hpp 
e5c2d3440a85ab2d2cae551ee4094cd965e38dfc 


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


Testing
---

sudo make check (mac os x, fedora 25)
internal CI


Thanks,

Andrei Budnik

Re: Review Request 60591: Optionally isolate only the agent network ports.

[James Peach]

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

(Updated Aug. 21, 2017, 8:36 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


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


Repository: mesos


Description
---

Normally, the `network/ports` isolator will kill any task that
listens on a port that it does not have resources for. However,
executors that are based on the libprocess API will always listen
on a port in the ephemeral range, and we want to make it possible
to use libprocess-based executors.

Added the `--container_ports_watch_resources_only` option to only
kill tasks when they listen on un-allocated ports within the port
range published by the agent resources. This still prevents port
collisions between tasks, but doesn't kill them just because the
executor is listening on a port.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
  src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
  src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 


Diff: https://reviews.apache.org/r/60591/diff/12/

Changes: https://reviews.apache.org/r/60591/diff/11-12/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60591: Optionally isolate only the agent network ports.

[James Peach]

> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 183 (patched)
> > 
> >
> > This method is only called by `NetworkPortsIsolatorProcess::create()` 
> > and its logic is pretty straightforward, so I would suggest to kill it and 
> > move its logic into `NetworkPortsIsolatorProcess::create()`.

Actually, keeping this in a helper function makes the caller significantly less 
complex. We can put the condition directly in the `if` statement and give it a 
meaningful name. The alternative is to have a number of similarly-named 
temporaries and a comment explaining what is going on. This is much cleaner.


> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 239 (patched)
> > 
> >
> > What if `ports.isSome()` is `false`? And what if ports resource is 
> > specified in `--resources` as `ports:[]`?

Good point. In that case, we need to specify an empty ports interval set. I 
added a new test to [r/60765](https://reviews.apache.org/r/60765/) to cover the 
case where we run a test with an empty ports resource.


> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 245-246 (patched)
> > 
> >
> > Won't agent listen on another available ephemeral port when it is 
> > restarted?

The port the agent listens on is never ephemeral; the default is 
`0.0.0.0:5051`. Thinking some more about this, it is quite difficult to prevent 
 a container listening on the agent port. If the agent is running, a container 
could only listen on this port if `SO_REUSEPORT` is being used. If the agent 
isn't running, then we could not prevent a container taking this port and 
thereby preventing the agent starting.


> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/flags.cpp
> > Lines 1012-1018 (patched)
> > 
> >
> > So by default this flag is not enabled, that means any libprocess-based 
> > exectuors (e.g., command executor and default executor) will be killed 
> > since they will listen on ephemeral port?

Yes, that's correct.


- James


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


On July 3, 2017, 10:30 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> ---
> 
> (Updated July 3, 2017, 10:30 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the `network/ports` isolator will kill any task that
> listens on a port that it does not have resources for. However,
> executors that are based on the libprocess API will always listen
> on a port in the ephemeral range, and we want to make it possible
> to use libprocess-based executors.
> 
> Added the `--container_ports_watch_resources_only` option to only
> kill tasks when they listen on un-allocated ports within the port
> range published by the agent resources. This still prevents port
> collisions between tasks, but doesn't kill them just because the
> executor is listening on a port.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
>   src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
>   src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 
> 
> 
> Diff: https://reviews.apache.org/r/60591/diff/11/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61791: Raised the logging level of some (health) check messages.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61791, 61766, 61697]

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

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
  File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
  File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 
25: ordinal not in range(128)

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/260/console

- Mesos Reviewbot Windows


On Aug. 21, 2017, 12:46 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61791/
> ---
> 
> (Updated Aug. 21, 2017, 12:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some users pointed out that always logging the result of (health)
> checks makes debugging problems easier.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61791/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61620: Tracked successful task fetches rather than total.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61620]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 21, 2017, 12:10 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61620/
> ---
> 
> (Updated Aug. 21, 2017, 12:10 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7888
> https://issues.apache.org/jira/browse/MESOS-7888
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the fetcher task metrics, track the number of times we
> successfully fetch all the task URIs rather than the total
> number of times we try to fetch task URIs. The success count
> is easier to observe and explain, and is consistent with the
> metrics nomenclature for other pending metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md d1f64d46b22e79c16a680a2e7fff9a01202c5457 
>   src/slave/containerizer/fetcher.cpp 
> fdeb9de346534fa7e223f90db9ba17f77a81611f 
>   src/slave/containerizer/fetcher_process.hpp 
> 36010e3b3f2bd5b67272baecb490dd2a39a4974b 
>   src/tests/fetcher_tests.cpp 01da9fef8e6766cc550d6dc5fe595de86906fb54 
> 
> 
> Diff: https://reviews.apache.org/r/61620/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61620: Tracked successful task fetches rather than total.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Aug. 21, 2017, 12:10 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61620/
> ---
> 
> (Updated Aug. 21, 2017, 12:10 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7888
> https://issues.apache.org/jira/browse/MESOS-7888
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the fetcher task metrics, track the number of times we
> successfully fetch all the task URIs rather than the total
> number of times we try to fetch task URIs. The success count
> is easier to observe and explain, and is consistent with the
> metrics nomenclature for other pending metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md d1f64d46b22e79c16a680a2e7fff9a01202c5457 
>   src/slave/containerizer/fetcher.cpp 
> fdeb9de346534fa7e223f90db9ba17f77a81611f 
>   src/slave/containerizer/fetcher_process.hpp 
> 36010e3b3f2bd5b67272baecb490dd2a39a4974b 
>   src/tests/fetcher_tests.cpp 01da9fef8e6766cc550d6dc5fe595de86906fb54 
> 
> 
> Diff: https://reviews.apache.org/r/61620/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 61791: Raised the logging level of some (health) check messages.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.


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


Repository: mesos


Description
---

Some users pointed out that always logging the result of (health)
checks makes debugging problems easier.


Diffs
-

  src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 


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


Testing
---

`make tests` on GNU/Linux


Thanks,

Gastón Kleiman

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Gastón Kleiman]

> On Aug. 21, 2017, 6:10 p.m., Greg Mann wrote:
> > src/checks/checker_process.cpp
> > Lines 149-150 (patched)
> > 
> >
> > This seems more concise to me, and just as readable:
> > 
> > s/stdoutReceived/stdout/
> > s/stderrReceived/stderr/
> > 
> > what do you think?

I associate `stdout` and `stderr` with the global variables from `stdio.h`, 
which are used for the stdout/stderr of the *current* process.

And apparently I am not the only one, for instance vim highlights them 
differently.

I checked with Greg on Slack and he's ok with keeping the current names, so I'm 
dropping this issue.


- Gastón


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


On Aug. 21, 2017, 7:08 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/4/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61620: Tracked successful task fetches rather than total.

[James Peach]

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

(Updated Aug. 21, 2017, 7:10 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased and addressed review feedback.


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


Repository: mesos


Description
---

In the fetcher task metrics, track the number of times we
successfully fetch all the task URIs rather than the total
number of times we try to fetch task URIs. The success count
is easier to observe and explain, and is consistent with the
metrics nomenclature for other pending metrics.


Diffs (updated)
-

  docs/monitoring.md d1f64d46b22e79c16a680a2e7fff9a01202c5457 
  src/slave/containerizer/fetcher.cpp fdeb9de346534fa7e223f90db9ba17f77a81611f 
  src/slave/containerizer/fetcher_process.hpp 
36010e3b3f2bd5b67272baecb490dd2a39a4974b 
  src/tests/fetcher_tests.cpp 01da9fef8e6766cc550d6dc5fe595de86906fb54 


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

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


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Gastón Kleiman]

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

(Updated Aug. 21, 2017, 7:08 p.m.)


Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.


Changes
---

Addressed Greg's comments.


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


Repository: mesos


Description
---

This patch makes the default executor include the output of the COMMAND
(health) checks in its logs.


Diffs (updated)
-

  src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 


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

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


Testing
---

Manual tests.


Thanks,

Gastón Kleiman

Re: Review Request 61220: Added MESOS-5116 to the CHANGELOG.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61220]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 21, 2017, 10:57 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61220/
> ---
> 
> (Updated Aug. 21, 2017, 10:57 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Kapil Arya, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added MESOS-5116 (XFS disk isolator support for no-enforce mode) to
> the CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 907623f46e5e574a27e616ba8443b4b2d183de89 
>   docs/configuration.md c149339c0847f4fbb9ab13918cbf26b815a46f6d 
>   docs/upgrades.md 349d8a144097d9be354a95c904ab7209ae3cc442 
> 
> 
> Diff: https://reviews.apache.org/r/61220/diff/4/
> 
> 
> Testing
> ---
> 
> Generated website and verified correct links and content.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61620: Tracked successful task fetches rather than total.

[Jiang Yan Xu]

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




src/slave/containerizer/fetcher.cpp
Line 575 (original), 573 (patched)


`onAny` takes a `void(const Future&)` callback so this looks odd, even 
thought it probably compiles due to the defer wrapper.

Since this has a `then` continuation below, can we just put it there?


- Jiang Yan Xu


On Aug. 14, 2017, 10:01 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61620/
> ---
> 
> (Updated Aug. 14, 2017, 10:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7888
> https://issues.apache.org/jira/browse/MESOS-7888
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the fetcher task metrics, track the number of times we
> successfully fetch all the task URIs rather than the total
> number of times we try to fetch task URIs. The success count
> is easier to observe and explain, and is consistent with the
> metrics nomenclature for other pending metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md d1f64d46b22e79c16a680a2e7fff9a01202c5457 
>   src/slave/containerizer/fetcher.cpp 
> fdeb9de346534fa7e223f90db9ba17f77a81611f 
>   src/slave/containerizer/fetcher_process.hpp 
> 36010e3b3f2bd5b67272baecb490dd2a39a4974b 
>   src/tests/fetcher_tests.cpp 01da9fef8e6766cc550d6dc5fe595de86906fb54 
> 
> 
> Diff: https://reviews.apache.org/r/61620/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Greg Mann]

> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 644-650 (patched)
> > 
> >
> > I'd rather `VLOG(1)` here. It seems we do `LOG(INFO)` for important 
> > messages in this file (3 occurences). It is also consistent with other 
> > checks.

IMO, in this case it will be more useful for our users if we include this in 
non-verbose logs. Using VLOG would mean that in order to get the output, a user 
would likely need to pass MESOS_VERBOSE=true and GLOG_v=2 env vars to the 
executor. If a bug occurs and reproducing the issue is difficult, then it could 
be hard for the user to get the relevant check logs.


- Greg


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


On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 18, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/3/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Greg Mann]

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




src/checks/checker_process.cpp
Lines 143-146 (patched)


Could you add a note here with a link to MESOS-7903, saying that we would 
like to refactor this into a common helper?



src/checks/checker_process.cpp
Lines 149-150 (patched)


This seems more concise to me, and just as readable:

s/stdoutReceived/stdout/
s/stderrReceived/stderr/

what do you think?



src/checks/checker_process.cpp
Lines 161 (patched)


Does `return Error(records.error());` not work here? And below.



src/checks/checker_process.cpp
Lines 588 (patched)


s/timed out/timed-out/


- Greg Mann


On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 18, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/3/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61220: Added MESOS-5116 to the CHANGELOG.

[James Peach]

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

(Updated Aug. 21, 2017, 5:57 p.m.)


Review request for mesos, Anand Mazumdar, Kapil Arya, and Jiang Yan Xu.


Repository: mesos


Description
---

Added MESOS-5116 (XFS disk isolator support for no-enforce mode) to
the CHANGELOG.


Diffs (updated)
-

  CHANGELOG 907623f46e5e574a27e616ba8443b4b2d183de89 
  docs/configuration.md c149339c0847f4fbb9ab13918cbf26b815a46f6d 
  docs/upgrades.md 349d8a144097d9be354a95c904ab7209ae3cc442 


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

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


Testing
---

Generated website and verified correct links and content.


Thanks,

James Peach

Re: Review Request 61220: Added MESOS-5116 to the CHANGELOG.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Would you mind rebasing it? Sorry it doesn't apply cleanly anymore due to the 
delay...


CHANGELOG
Lines 29-31 (patched)


The list of features are orderer by their JIRA number. Reorder it?


- Jiang Yan Xu


On Aug. 16, 2017, 3:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61220/
> ---
> 
> (Updated Aug. 16, 2017, 3:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Kapil Arya, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added MESOS-5116 (XFS disk isolator support for no-enforce mode) to
> the CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG abb0e3bea95648b91605cec37b21f079d2b15b41 
>   docs/configuration.md 6ab2d1a06de9e4fff9e6769d2b2cbd05e8f3d17a 
>   docs/upgrades.md 349d8a144097d9be354a95c904ab7209ae3cc442 
> 
> 
> Diff: https://reviews.apache.org/r/61220/diff/3/
> 
> 
> Testing
> ---
> 
> Generated website and verified correct links and content.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60592: Configure the `network/ports` isolator watch interval.

[James Peach]

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

(Updated Aug. 21, 2017, 3:50 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

Added the `--container_ports_watch_interval` option to tune the
interval at which the `network/ports` isolator scans for rogue
listening ports.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
  src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
  src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 


Diff: https://reviews.apache.org/r/60592/diff/11/

Changes: https://reviews.apache.org/r/60592/diff/10-11/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 61766: Made the output handling of TCP and HTTP checks consistent.

[Alexander Rukletsov]

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




src/checks/checker_process.cpp
Lines 921-924 (original), 921-924 (patched)


Additionally `VLOG(1)` stderr?


- Alexander Rukletsov


On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> ---
> 
> (Updated Aug. 19, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the output handling of TCP and HTTP checks consistent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61766/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61766: Made the output handling of TCP and HTTP checks consistent.

[Alexander Rukletsov]

> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 913-920 (original), 913-920 (patched)
> > 
> >
> > Let's print stderr regardless of the retcode, what you now do with the 
> > command checks. It is probably not much value to do so here, but it is 
> > _consistent_ : ).

Thinking more about it, I lean towards your solution and not print anything 
unless there was a curl failure. HTTP checks are actually not meant to be 
processes, so printing `curl`'s stdout and stderr is not necessary.


> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 1071-1076 (original), 1075-1080 (patched)
> > 
> >
> > Let's print it regardless of the retcode, what you now do with the 
> > command checks. It is probably not much value to do so for the tcp checker, 
> > but it is _consistent_ : ).

Thinking more about it, I lean towards your solution and not print anything 
unless there was a `tcp-connect` failure. TCP checks are actually not meant to 
be processes, so printing `tcp-connect`'s stdout and stderr is not necessary.


- Alexander


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


On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> ---
> 
> (Updated Aug. 19, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the output handling of TCP and HTTP checks consistent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61766/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Alexander Rukletsov]

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




src/checks/checker_process.cpp
Lines 644-650 (patched)


I'd rather `VLOG(1)` here. It seems we do `LOG(INFO)` for important 
messages in this file (3 occurences). It is also consistent with other checks.


- Alexander Rukletsov


On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 18, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/3/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61766: Made the output handling of TCP and HTTP checks consistent.

[Alexander Rukletsov]

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




src/checks/checker_process.cpp
Lines 913-920 (original), 913-920 (patched)


Let's print stderr regardless of the retcode, what you now do with the 
command checks. It is probably not much value to do so here, but it is 
_consistent_ : ).



src/checks/checker_process.cpp
Lines 916-919 (original), 916-919 (patched)


Looking at this, I'm not sure we should return a failure here: `stderr` is 
not critical for getting the HTTP code, so maybe log warning here? This is also 
consistent with what you now do for nested command checks.



src/checks/checker_process.cpp
Lines 1071-1076 (original), 1075-1080 (patched)


Let's print it regardless of the retcode, what you now do with the command 
checks. It is probably not much value to do so for the tcp checker, but it is 
_consistent_ : ).


- Alexander Rukletsov


On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> ---
> 
> (Updated Aug. 19, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the output handling of TCP and HTTP checks consistent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61766/diff/1/
> 
> 
> Testing
> ---
> 
> `make tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61666: Added a test to verify filtering of resource reservations on agent.

[Alexander Rojas]

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




src/tests/slave_authorization_tests.cpp
Lines 106 (patched)


_Only one default user…_



src/tests/slave_authorization_tests.cpp
Lines 164 (patched)


I would rather have `acl->mutable_roles()->add_value(role_name)`



src/tests/slave_authorization_tests.cpp
Lines 195 (patched)


can you give it a better name to this role, even _test-role_ would do, but 
if you look at api tests we use other things like _superhero_ or _muggle_.


- Alexander Rojas


On Aug. 15, 2017, 7:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61666/
> ---
> 
> (Updated Aug. 15, 2017, 7:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7892
> https://issues.apache.org/jira/browse/MESOS-7892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends `SlaveAuthorizerTest.FilterStateEndpoint` test to
> check that `/state` endpoint shows resource reservations only for
> allowed principal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp 
> 536a8efda0cc1ba08285c379b01af6ec64a82117 
> 
> 
> Diff: https://reviews.apache.org/r/61666/diff/1/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61763: Added heartbeat interval for V1 Operator API.

[Mesos Reviewbot Windows]

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



Failed to apply patch!

Reviews applied: [61759, 61763]

Logs available here: http://104.210.40.105/logs/master/61763

- Mesos Reviewbot Windows


On Aug. 18, 2017, 11:40 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61763/
> ---
> 
> (Updated Aug. 18, 2017, 11:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the heartbeat interval parameter to the SUBSCRIBED event for
> V1 operator API. Updated the heartbeat test case to check the
> heartbeat interval.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7dc5881b843addc2ec59fadb45f31049c7e5a588 
>   include/mesos/v1/master/master.proto 
> db19c5ccc8aca78f901e72f3caf6d46761270bdd 
>   src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8 
>   src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6 
> 
> 
> Diff: https://reviews.apache.org/r/61763/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61759: Updated the docs for V1 HEARTBEAT Event.

[Mesos Reviewbot Windows]

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



Failed to apply patch!

Reviews applied: [61759]

Logs available here: http://104.210.40.105/logs/master/61759

- Mesos Reviewbot Windows


On Aug. 18, 2017, 10:04 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61759/
> ---
> 
> (Updated Aug. 18, 2017, 10:04 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7695
> https://issues.apache.org/jira/browse/MESOS-7695
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the docs for V1 HEARTBEAT Event.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md c56f3b567b07b73602d6cbd7666fc2a31b2475c9 
> 
> 
> Diff: https://reviews.apache.org/r/61759/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61189: Added authorization for V1 events.

[Mesos Reviewbot Windows]

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



Failed to apply patch!

Reviews applied: [61262, 61189]

Logs available here: http://104.210.40.105/logs/master/61189

- Mesos Reviewbot Windows


On Aug. 19, 2017, 3:33 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> ---
> 
> (Updated Aug. 19, 2017, 3:33 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8 
>   src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604 
>   src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 
>   src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> GLOG_v=2 ./bin/mesos-tests.sh 
> --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" 
> --verbose --gtest_repeat=100 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60903: Added the `network/ports` isolator to the Mesos containerizer.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On July 17, 2017, 7:44 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60903/
> ---
> 
> (Updated July 17, 2017, 7:44 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the `network/ports` isolator to the set of isolators known
> to the Mesos containerizer so  that it can be enabled by operators.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ff192bb085f3554ad1b1f20fb73bf827bf04ef13 
> 
> 
> Diff: https://reviews.apache.org/r/60903/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60766: Ignored containers that join CNI networks.

[Qian Zhang]

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




src/slave/containerizer/mesos/containerizer.cpp
Line 244 (original), 245 (patched)


So here we only count `network/cni` isolator and `network/port_mapping` 
isolator, either of them (but not both of them) can work with `network/ports` 
isolator. Can you please also update the comments accordingly?



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 308-311 (original), 313-323 (patched)


I think we only need to do this check for top-level container, but not for 
nested container since nested container always share network namespace with its 
parent. So we may need to add `!containerId.has_parent()` into the condition of 
the first `if`.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 392-402 (patched)


Can we check `state->executor_info().container().network_infos()` rather 
than checking CNI container dir?


- Qian Zhang


On July 29, 2017, 8:01 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> ---
> 
> (Updated July 29, 2017, 8:01 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 5772421c3078d36225b946a5286b8c1bf2f007e8 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60766/diff/8/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60591: Optionally isolate only the agent network ports.

[Qian Zhang]

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




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


This method is only called by `NetworkPortsIsolatorProcess::create()` and 
its logic is pretty straightforward, so I would suggest to kill it and move its 
logic into `NetworkPortsIsolatorProcess::create()`.



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


What if `ports.isSome()` is `false`? And what if ports resource is 
specified in `--resources` as `ports:[]`?



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 245-246 (patched)


Won't agent listen on another available ephemeral port when it is restarted?



src/slave/flags.cpp
Lines 1012-1018 (patched)


So by default this flag is not enabled, that means any libprocess-based 
exectuors (e.g., command executor and default executor) will be killed since 
they will listen on ephemeral port?


- Qian Zhang


On July 3, 2017, 6:30 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> ---
> 
> (Updated July 3, 2017, 6:30 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the `network/ports` isolator will kill any task that
> listens on a port that it does not have resources for. However,
> executors that are based on the libprocess API will always listen
> on a port in the ephemeral range, and we want to make it possible
> to use libprocess-based executors.
> 
> Added the `--container_ports_watch_resources_only` option to only
> kill tasks when they listen on un-allocated ports within the port
> range published by the agent resources. This still prevents port
> collisions between tasks, but doesn't kill them just because the
> executor is listening on a port.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
>   src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
>   src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 
> 
> 
> Diff: https://reviews.apache.org/r/60591/diff/10/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>