date:20170911

Re: Review Request 62230: Avoid GC pruning events from blocking other processes.

2017-09-11 Thread Chun-Hung Hsiao


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

(Updated Sept. 12, 2017, 2:37 a.m.)


Review request for mesos, Benjamin Mahler, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed xujyan's comment.


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


Repository: mesos


Description (updated)
---

This patch dispatches `rmdirs` to a single executor instead of multiple
`AsyncExecutor`s such that heavy-duty pruning events won't occupy all
worker threads and block other actors.


Diffs (updated)
-

  src/slave/gc.cpp 83e4e2f3aba5c0d9900cf0beeea6e92320f889e7 
  src/slave/gc_process.hpp c383ce28411622692e42401c80e9443e7b1f5099 


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

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


Testing
---

sudo make test


Thanks,

Chun-Hung Hsiao

Re: Review Request 62003: Added `network/ports` isolator nested container tests.

2017-09-11 Thread James Peach



> On Sept. 6, 2017, 1 p.m., Qian Zhang wrote:
> > src/tests/containerizer/ports_isolator_tests.cpp
> > Lines 1051-1052 (patched)
> > 
> >
> > Can you please elaborate a bit about this? What do you mean for `the 
> > original nested container status gets swallowed`?
> 
> James Peach wrote:
> When we are not using nested containers, we get a 
> `REASON_CONTAINER_LIMITATION` status update which comes directly from the 
> isolator. However, when we use nested containers, we actually get a 
> `REASON_COMMAND_EXECUTOR_FAILED` reason; the task status that comes from the 
> isolator never goes anywhere.
> 
> Qian Zhang wrote:
> > However, when we use nested containers, we actually get a 
> REASON_COMMAND_EXECUTOR_FAILED reason; the task status that comes from the 
> isolator never goes anywhere.
> 
> Is it because there is only one nested container and when that nested 
> container is destroyed by Mesos containerizer, the default executor will know 
> it and terminate itself, so it has no chance to send 
> REASON_CONTAINER_LIMITATION status for that nested container before it's self 
> termination? If so, what if we launch a task group which has two tasks? when 
> one of the task is destroyed due to the limitation, will we get a 
> `REASON_CONTAINER_LIMITATION` status update for it?
> 
> James Peach wrote:
> I can reproduce this with `mesos-execute` so I filed 
> [MESOS-7963](https://issues.apache.org/jira/browse/MESOS-7963) to handle this.

The nested container status gets lost 
[here](https://github.com/apache/mesos/blob/master/src/slave/http.cpp#L2496), 
so I think the right approach is to always trigger the resource limitation on 
the root container.


- James


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


On Sept. 8, 2017, 10:44 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> ---
> 
> (Updated Sept. 8, 2017, 10:44 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 
> 
> 
> Diff: https://reviews.apache.org/r/62003/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62047: Allowed look up latest executor directory by virtual path.

2017-09-11 Thread Zhitao Li


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

(Updated Sept. 11, 2017, 11:18 p.m.)


Review request for mesos, Benjamin Mahler and Jason Lai.


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


Repository: mesos


Description
---

This patch added support to attach a virtual path
`/frameworks/{framework-id}/executors/{executor-id}/latest` to `Files` class
in agent, which can be used to navigate an executor's latest sandbox without
needing to know `work_dir` and `slave_id`.


Diffs
-

  src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
  src/slave/paths.cpp 08177bcef63b998f691be6893ab35891098a2886 
  src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd 
  src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 


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


Testing (updated)
---

Created new unit test to browse and read file through virtual sandbox path.


Thanks,

Zhitao Li

Re: Review Request 62047: Allowed look up latest executor directory by virtual path.

2017-09-11 Thread Zhitao Li


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

(Updated Sept. 11, 2017, 11:17 p.m.)


Review request for mesos, Benjamin Mahler and Jason Lai.


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


Repository: mesos


Description
---

This patch added support to attach a virtual path
`/frameworks/{framework-id}/executors/{executor-id}/latest` to `Files` class
in agent, which can be used to navigate an executor's latest sandbox without
needing to know `work_dir` and `slave_id`.


Diffs (updated)
-

  src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
  src/slave/paths.cpp 08177bcef63b998f691be6893ab35891098a2886 
  src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd 
  src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 


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

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


Testing
---

Ran a master/agent pair, launch a task with `mesos-execute` and use 
`files/browse` endpoint to browse the virtual path.


Thanks,

Zhitao Li

Re: Review Request 62040: Also log attached virtual path in agent.

2017-09-11 Thread Zhitao Li


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

(Updated Sept. 11, 2017, 11:16 p.m.)


Review request for mesos, Benjamin Mahler and Jason Lai.


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


Repository: mesos


Description
---

The same physical directory will be mounted to multiple virtual paths,
  so log which virtual path in agent log.


Diffs (updated)
-

  src/slave/constants.hpp 435c37e28987119c4ef43c9dad61ac052020acd8 
  src/slave/slave.hpp 7d07868451e93d34ba694d40216c1e4036fd4094 
  src/slave/slave.cpp 6d1516a5d5b5db684f79385e60d892ff75fd00fd 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 62230: Avoid GC pruning events from blocking other processes.

2017-09-11 Thread Jiang Yan Xu


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




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


I was suggesting using this: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/executor.hpp
 as a helper actor with the same lifecycle as the gc actor, and we dispatch all 
rmdirs onto that actor.

We did the previous patch precisely to avoid dispatching onto the gc actor 
itself because the rmdirs could block new task launches.


- Jiang Yan Xu


On Sept. 11, 2017, 2:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62230/
> ---
> 
> (Updated Sept. 11, 2017, 2:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7964
> https://issues.apache.org/jira/browse/MESOS-7964
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch dispatches GC pruning events to `GarbageCollectorProcess`
> instead of multiple `AsyncExecutor`s such that if multiple heavy-duty
> pruning events won't occupy all worker threads and block other actors.
> 
> 
> Diffs
> -
> 
>   src/slave/gc.cpp 83e4e2f3aba5c0d9900cf0beeea6e92320f889e7 
> 
> 
> Diff: https://reviews.apache.org/r/62230/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make test
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

2017-09-11 Thread Mesos Reviewbot Windows


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



FAIL: Some Mesos libprocess-tests failed.

Reviews applied: [62018, 62037]

All the build artifacts available 
[here](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62037).

Relevant logs:

 - libprocess-tests-stdout.log (full log 
[here](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62037/logs/libprocess-tests-stdout.log)):
```
[   OK ] LimiterTest.THREADSAFE_Acquire (8 ms)
[ RUN  ] LimiterTest.THREADSAFE_DiscardMiddle
[   OK ] LimiterTest.THREADSAFE_DiscardMiddle (2 ms)
[ RUN  ] LimiterTest.THREADSAFE_DiscardLast
[   OK ] LimiterTest.THREADSAFE_DiscardLast (2 ms)
[--] 3 tests from LimiterTest (68 ms total)

[--] 4 tests from LoopTest
[ RUN  ] LoopTest.Sync
[   OK ] LoopTest.Sync (1 ms)
[ RUN  ] LoopTest.Async
[   OK ] LoopTest.Async (2 ms)
[ RUN  ] LoopTest.DiscardIterate
[   OK ] LoopTest.DiscardIterate (1 ms)
[ RUN  ] LoopTest.DiscardBody
[   OK ] LoopTest.DiscardBody (3 ms)
[--] 4 tests from LoopTest (80 ms total)

[--] 9 tests from MetricsTest
[ RUN  ] MetricsTest.Counter
[   OK ] MetricsTest.Counter (3 ms)
[ RUN  ] MetricsTest.THREADSAFE_Gauge
[   OK ] MetricsTest.THREADSAFE_Gauge (4 ms)
[ RUN  ] MetricsTest.Statistics
[   OK ] MetricsTest.Statistics (6 ms)
[ RUN  ] MetricsTest.THREADSAFE_Snapshot
[   OK ] MetricsTest.THREADSAFE_Snapshot (63 ms)
[ RUN  ] MetricsTest.THREADSAFE_SnapshotTimeout
C:\mesos\mesos\3rdparty\libprocess\src\tests\metrics_tests.cpp(330): error: 
Failed to wait 15secs for response
```
 - libprocess-tests-stderr.log (full log 
[here](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62037/logs/libprocess-tests-stderr.log)):
```
```

- Mesos Reviewbot Windows


On Sept. 11, 2017, 2:59 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62037/
> ---
> 
> (Updated Sept. 11, 2017, 2:59 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change reduces the number of messages "WARNING: Logging
> before InitGoogleLogging() is written to STDERR".
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 3cb9077b9f39f69fdc152280e5b180b88e772648 
>   src/cli/execute.cpp e844fa46a43bdfb08246be929b7913a7997f18df 
>   src/cli/resolve.cpp b3cba87f61dbff0e99291775a4d0908bb35ce811 
>   src/examples/balloon_framework.cpp 1a97f56c3e217c58b408949306b92f19caa5f6c6 
>   src/examples/disk_full_framework.cpp 
> f9d5af5e45d5736581ce2c5395c741c995332136 
>   src/examples/dynamic_reservation_framework.cpp 
> bb6f58ba8fb87ae37d4ae971c192a204d5a656c5 
>   src/examples/long_lived_framework.cpp 
> 3de4a0280c41db5dcf656ac89cb4168060190108 
>   src/examples/no_executor_framework.cpp 
> 2ca240b4c998f0e33e7b09dbad9f4a7bfa0583cc 
>   src/examples/test_framework.cpp a6b38f02cc29682e3d3e1adf83baea061feda9e7 
>   src/examples/test_http_framework.cpp 
> 693dd47694678e7439f9d4ab0ff77b93950edf6e 
>   src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
>   src/slave/container_loggers/logrotate.cpp 
> 61484b18f4615e85925b26d999afb5ac3b7e32a5 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> fa0b29667258b6b6fb7381d8226b6dda0062629e 
>   src/usage/main.cpp 5ad4a3ba7959e4a9b3fa05f5a2f49b02f94a0cf0 
> 
> 
> Diff: https://reviews.apache.org/r/62037/diff/3/
> 
> 
> Testing
> ---
> 
> ```
> make check
> ```
> 
> Had an issue with ExamplesTest.DiskFullFramework that is not related.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 62230: Avoid GC pruning events from blocking other processes.

2017-09-11 Thread Chun-Hung Hsiao


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

Review request for mesos, Benjamin Mahler, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

This patch dispatches GC pruning events to `GarbageCollectorProcess`
instead of multiple `AsyncExecutor`s such that if multiple heavy-duty
pruning events won't occupy all worker threads and block other actors.


Diffs
-

  src/slave/gc.cpp 83e4e2f3aba5c0d9900cf0beeea6e92320f889e7 


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


Testing
---

sudo make test


Thanks,

Chun-Hung Hsiao

Re: Review Request 61473: Do not kill non partition aware tasks.

2017-09-11 Thread Megha Sharma



> On Aug. 30, 2017, 1:05 a.m., Jiang Yan Xu wrote:
> > src/master/master.cpp
> > Lines 8948-8950 (patched)
> > 
> >
> > If the framework is not partition aware, the `update` will already have 
> > a `TASK_LOST` state right?

Good point! Fixed in the new update.


- Megha


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


On Sept. 11, 2017, 9:23 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Sept. 11, 2017, 9:23 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including the
> ones that are still registered which was problematic because the offer
> from this agent could still go to the same framework which could then
> launch new tasks. The agent would then receive tasks of the same
> framework and ignore them because it thinks the framework is shutting
> down. The framework is not shutting down of course, so from the master
> and the scheduler’s perspective the task is pending in STAGING forever
> until the next agent reregistration, which could happen much later.
> This commit fixes the problem by not shutting down the non-partition
> aware frameworks on such an agent.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 28d0393fb5962df4d731521265efd81a54e1e655 
>   src/master/master.hpp 05f88111afb4fa0e2baf57106e1479914c16a113 
>   src/master/master.cpp 6d84a26bff970b842b58dfb69dbf232ba5c16a20 
>   src/tests/partition_tests.cpp 0886f4890ac3fec6f38146946892769a99c3e68f 
> 
> 
> Diff: https://reviews.apache.org/r/61473/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 61473: Do not kill non partition aware tasks.

2017-09-11 Thread Megha Sharma


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

(Updated Sept. 11, 2017, 9:23 p.m.)


Review request for mesos, Vinod Kone and Jiang Yan Xu.


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


Repository: mesos


Description
---

Master will not kill the tasks for non-Partition aware frameworks
when an unreachable agent re-registers with the master.
Master used to send a ShutdownFrameworkMessages to the agent
to kill the tasks from non partition aware frameworks including the
ones that are still registered which was problematic because the offer
from this agent could still go to the same framework which could then
launch new tasks. The agent would then receive tasks of the same
framework and ignore them because it thinks the framework is shutting
down. The framework is not shutting down of course, so from the master
and the scheduler’s perspective the task is pending in STAGING forever
until the next agent reregistration, which could happen much later.
This commit fixes the problem by not shutting down the non-partition
aware frameworks on such an agent.


Diffs (updated)
-

  src/master/http.cpp 28d0393fb5962df4d731521265efd81a54e1e655 
  src/master/master.hpp 05f88111afb4fa0e2baf57106e1479914c16a113 
  src/master/master.cpp 6d84a26bff970b842b58dfb69dbf232ba5c16a20 
  src/tests/partition_tests.cpp 0886f4890ac3fec6f38146946892769a99c3e68f 


Diff: https://reviews.apache.org/r/61473/diff/7/

Changes: https://reviews.apache.org/r/61473/diff/6-7/


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 62214: Added JavaScript linter.

2017-09-11 Thread Benjamin Mahler

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

Can you add Kevin Klues to the review and ask him to take a look at the virtual 
env bootstrapping? I'm assuming you based this on his one for the CLI?

src/webui/master/static/.eslintrc.js
Lines 6 (patched)

Does this run successfully on the webui? Can you document for posterity how 
this config was produced?

src/webui/master/static/.gitignore
Lines 1 (patched)

Ditto here re: serving the file over HTTP.

src/webui/master/static/bootstrap
Lines 3-4 (patched)

I think the master serves all assets under static:

https://github.com/apache/mesos/blob/297b7042a7ef65aafca40832b5f8736e27e26ed2/src/master/master.cpp#L1122

Which means that if this file is here it's served by the master.

support/mesos-style.py
Lines 298-302 (patched)

Stale copy paste

support/mesos-style.py
Lines 307-311 (patched)

Similarly to my comment below, if we had some virtual env abstraction this 
could be running something within in:

```
virtualenv.run_within(['eslint', ...]);
```

support/mesos-style.py
Lines 309 (patched)

Is this applying a lint on the whole file or just the diff? Is it possible 
to do just the diff with eslint?

If not, we'll need to make sure the lint result is clean before we commit 
this, right?

support/mesos-style.py
Lines 332-334 (patched)

Copy paste?

support/mesos-style.py
Lines 344-361 (patched)

Can you document that we build the virtualenv by running bootstrap?

It will be unfortunate to have the code here and in PyLinter diverge, have 
you considered adding some kind of VirtualEnv class or making these standalone 
virtualenv functions here for them to both reuse?

support/mesos-style.py
Lines 343-345 (original)

What happened here? Should this be a different patch?

- Benjamin Mahler

On Sept. 11, 2017, 9:49 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62214/
> ---
> 
> (Updated Sept. 11, 2017, 9:49 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7924
> https://issues.apache.org/jira/browse/MESOS-7924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This linter runs when changes on a JavaScript file are being committed.
> The linter used is ESLint with a configuration based on our current JS
> code base. The linter and its dependencies (i.e. Node.js) are installed
> in a virtual environment using Virtualenv and then Nodeenv.
> 
> 
> Diffs
> -
> 
>   src/.gitignore PRE-CREATION 
>   src/webui/master/static/.eslintrc.js PRE-CREATION 
>   src/webui/master/static/.gitignore PRE-CREATION 
>   src/webui/master/static/bootstrap PRE-CREATION 
>   src/webui/master/static/pip-requirements.txt PRE-CREATION 
>   support/mesos-style.py cf37d9f4da4ab90b92f0136a1dcd5dd8bbae5785 
> 
> 
> Diff: https://reviews.apache.org/r/62214/diff/1/
> 
> 
> Testing
> ---
> 
> Following this commit, I have tried to commit a change on a JavaScript file 
> and checked that ESLinter was correctly invoked.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62003: Added `network/ports` isolator nested container tests.

2017-09-11 Thread James Peach



> On Sept. 6, 2017, 1 p.m., Qian Zhang wrote:
> > src/tests/containerizer/ports_isolator_tests.cpp
> > Lines 1051-1052 (patched)
> > 
> >
> > Can you please elaborate a bit about this? What do you mean for `the 
> > original nested container status gets swallowed`?
> 
> James Peach wrote:
> When we are not using nested containers, we get a 
> `REASON_CONTAINER_LIMITATION` status update which comes directly from the 
> isolator. However, when we use nested containers, we actually get a 
> `REASON_COMMAND_EXECUTOR_FAILED` reason; the task status that comes from the 
> isolator never goes anywhere.
> 
> Qian Zhang wrote:
> > However, when we use nested containers, we actually get a 
> REASON_COMMAND_EXECUTOR_FAILED reason; the task status that comes from the 
> isolator never goes anywhere.
> 
> Is it because there is only one nested container and when that nested 
> container is destroyed by Mesos containerizer, the default executor will know 
> it and terminate itself, so it has no chance to send 
> REASON_CONTAINER_LIMITATION status for that nested container before it's self 
> termination? If so, what if we launch a task group which has two tasks? when 
> one of the task is destroyed due to the limitation, will we get a 
> `REASON_CONTAINER_LIMITATION` status update for it?

I can reproduce this with `mesos-execute` so I filed 
[MESOS-7963](https://issues.apache.org/jira/browse/MESOS-7963) to handle this.


- James


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


On Sept. 8, 2017, 10:44 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> ---
> 
> (Updated Sept. 8, 2017, 10:44 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 
> 
> 
> Diff: https://reviews.apache.org/r/62003/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

2017-09-11 Thread James Peach



> On Sept. 8, 2017, 7:43 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 112 (original), 112 (patched)
> > 
> >
> > Why do we need a `for` loop like this? I think the previous `while` 
> > loop is good enough.

If we want to depend on `errno`, we need to guarantee that it is `0` before we 
call the `readddir`. Since there is non-trivial logic inside the loop, it is 
difficult to be confident that nothing we call is going to change it, 
particularly if this code is ever updated.


> On Sept. 8, 2017, 7:43 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 124 (original), 125 (patched)
> > 
> >
> > This seems too strict, since here we are already going to return an 
> > error out, I think we can simply ignore the error of `closedir()`.

The only way `closedir` can fail is if we give it a bad pointer, so if we check 
this I think `CHECK` is appropriate. I don't understand the rationale of 
wanting to check the `readdir` but not the `closedir` since the error 
conditions for these are identical.


> On Sept. 8, 2017, 7:43 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 144 (patched)
> > 
> >
> > Better to change this to:
> > ```
> >   if (closedir(dir) == -1) {
> > return ErrnoError("Failed to close directory '" + fdPath + "'");
> >   }
> > ```
> > This is also aligned with how you handle the error of `opendir()` in 
> > the above.

I don't understand why we want to check the error return for this one but not 
the others. Since the `closedir` can't fail, IMHO we should just either assert 
or ignore it consistently.


- James


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


On Sept. 7, 2017, 9:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 7, 2017, 9:50 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
> ---
> 
> Implemented network ports isolator socket utilities to find the
> inode numbers of all the listening sockets, and the inodes of the
> open sockets for a process. Together, these utilities can tell us
> which sockets a process is listening on.
> 
> 
> Diffs
> -
> 
>   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/60495/diff/15/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62214: Added JavaScript linter.

2017-09-11 Thread Mesos Reviewbot Windows


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



FAIL: Some Mesos tests failed. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62214/logs/mesos-tests-stdout.log
 and 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62214/logs/mesos-tests-stderr.log
 for any relevant errors

Reviews applied: [62214]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62214/logs

- Mesos Reviewbot Windows


On Sept. 11, 2017, 9:49 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62214/
> ---
> 
> (Updated Sept. 11, 2017, 9:49 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7924
> https://issues.apache.org/jira/browse/MESOS-7924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This linter runs when changes on a JavaScript file are being committed.
> The linter used is ESLint with a configuration based on our current JS
> code base. The linter and its dependencies (i.e. Node.js) are installed
> in a virtual environment using Virtualenv and then Nodeenv.
> 
> 
> Diffs
> -
> 
>   src/.gitignore PRE-CREATION 
>   src/webui/master/static/.eslintrc.js PRE-CREATION 
>   src/webui/master/static/.gitignore PRE-CREATION 
>   src/webui/master/static/bootstrap PRE-CREATION 
>   src/webui/master/static/pip-requirements.txt PRE-CREATION 
>   support/mesos-style.py cf37d9f4da4ab90b92f0136a1dcd5dd8bbae5785 
> 
> 
> Diff: https://reviews.apache.org/r/62214/diff/1/
> 
> 
> Testing
> ---
> 
> Following this commit, I have tried to commit a change on a JavaScript file 
> and checked that ESLinter was correctly invoked.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

2017-09-11 Thread Armand Grillet


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

(Updated Sept. 11, 2017, 2:59 p.m.)


Review request for mesos, Andrei Budnik and Alexander Rukletsov.


Changes
---

Resolved issues.


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


Repository: mesos


Description
---

This change reduces the number of messages "WARNING: Logging
before InitGoogleLogging() is written to STDERR".


Diffs (updated)
-

  src/checks/tcp_connect.cpp 3cb9077b9f39f69fdc152280e5b180b88e772648 
  src/cli/execute.cpp e844fa46a43bdfb08246be929b7913a7997f18df 
  src/cli/resolve.cpp b3cba87f61dbff0e99291775a4d0908bb35ce811 
  src/examples/balloon_framework.cpp 1a97f56c3e217c58b408949306b92f19caa5f6c6 
  src/examples/disk_full_framework.cpp f9d5af5e45d5736581ce2c5395c741c995332136 
  src/examples/dynamic_reservation_framework.cpp 
bb6f58ba8fb87ae37d4ae971c192a204d5a656c5 
  src/examples/long_lived_framework.cpp 
3de4a0280c41db5dcf656ac89cb4168060190108 
  src/examples/no_executor_framework.cpp 
2ca240b4c998f0e33e7b09dbad9f4a7bfa0583cc 
  src/examples/test_framework.cpp a6b38f02cc29682e3d3e1adf83baea061feda9e7 
  src/examples/test_http_framework.cpp 693dd47694678e7439f9d4ab0ff77b93950edf6e 
  src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
  src/slave/container_loggers/logrotate.cpp 
61484b18f4615e85925b26d999afb5ac3b7e32a5 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
fa0b29667258b6b6fb7381d8226b6dda0062629e 
  src/usage/main.cpp 5ad4a3ba7959e4a9b3fa05f5a2f49b02f94a0cf0 


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

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


Testing
---

```
make check
```

Had an issue with ExamplesTest.DiskFullFramework that is not related.


Thanks,

Armand Grillet

Re: Review Request 62018: Updated logging::initialize to make flags optional.

2017-09-11 Thread Armand Grillet



> On Sept. 8, 2017, 12:30 p.m., Andrei Budnik wrote:
> > src/logging/logging.cpp
> > Line 156 (original), 165 (patched)
> > 
> >
> > What will be the value of `FLAGS_logtostderr` if `_flags` is `None`? 
> > Does it mean that messages via `LOG` won't be written to `stdout`/`stderr`?

By default, `FLAGS_logtostderr` value is 0. Thus we will not log to stderr 
instead of log files.


- Armand


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


On Sept. 6, 2017, 9:48 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62018/
> ---
> 
> (Updated Sept. 6, 2017, 9:48 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Thiw will be used to initialize logging in the main functions
> that do not use mesos::internal::logging::Flags.
> 
> 
> Diffs
> -
> 
>   src/logging/logging.hpp 0e448f25de6c66d772f5bc481fbaf4aa493fd8af 
>   src/logging/logging.cpp 70d66a5c396f709e8f27ad0d51315ed6d257f73b 
> 
> 
> Diff: https://reviews.apache.org/r/62018/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> $ make check
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62213: Send TASK_STARTING from the built-in executors. [2/2]

2017-09-11 Thread Mesos Reviewbot Windows


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



FAIL: Some Mesos tests failed. Please check 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62213/logs/mesos-tests-stdout.log
 and 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62213/logs/mesos-tests-stderr.log
 for any relevant errors

Reviews applied: [62212, 62213]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62213/logs

- Mesos Reviewbot Windows


On Sept. 11, 2017, 9:17 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62213/
> ---
> 
> (Updated Sept. 11, 2017, 9:17 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix unit tests for previous commit.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 0902a66772d178505e83b81878b7878a45c7bb25 
>   src/tests/check_tests.cpp cc7be317030d147c5617e784a50e42b0365b9df6 
>   src/tests/command_executor_tests.cpp 
> f706f55b5bfab824268498d95d775b216561cd66 
>   src/tests/container_logger_tests.cpp 
> 97e79792d3ea8023890ad2a705db47f2aeb419cf 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp 
> 38fef2d5f677f768a0533d1ac085b1197b3b764d 
>   src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
> 0030cd1e1f73422bef806bbc0453134e3d7840d8 
>   src/tests/default_executor_tests.cpp 
> 186b8333c02ba3b9257e19437c6d689761085362 
>   src/tests/disk_quota_tests.cpp 3bf0508238a228d86737d6cc899fa68e2046f2e2 
>   src/tests/fault_tolerance_tests.cpp 
> 5ac38a897fefb6f40d69ca6e27a6f23176e42d36 
>   src/tests/health_check_tests.cpp 2c43241a5cc245a1797ea16c68c94638cec6803d 
>   src/tests/master_tests.cpp 59fbad468ae671a8fd39457632262015989f26c2 
>   src/tests/master_validation_tests.cpp 
> 710b25c1afdff4de7b2eb9b8a38f6856c373fb3c 
>   src/tests/oversubscription_tests.cpp 
> a4c4a6083a2cb55e2d69eba1719555a7a13ee8fb 
>   src/tests/partition_tests.cpp 0886f4890ac3fec6f38146946892769a99c3e68f 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 7a24bf48e9d84bf31ad85397ac3e1b4e566cbc2c 
>   src/tests/persistent_volume_tests.cpp 
> 3e1d1fe468298186347b07b5882973c5a16231a3 
>   src/tests/reconciliation_tests.cpp 728961874239c7e5ce8accbaeb2a1c86a73a4f0f 
>   src/tests/reservation_endpoints_tests.cpp 
> 3278732cb130c73be0f20c0204eddaee05123ff9 
>   src/tests/role_tests.cpp fc4c017dc59ab44a0ce0ea46f02eb6e1706ffb72 
>   src/tests/scheduler_tests.cpp 21f8825c214bb0c331f66bfb9b12a354d6673a43 
>   src/tests/slave_authorization_tests.cpp 
> 30eceae0920351cffc9c3393c6d08917c4041c1a 
>   src/tests/slave_recovery_tests.cpp 0e46748be266809c413fb10fc447516c51504fce 
>   src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 
>   src/tests/teardown_tests.cpp 8834333bbcdb6a04a95a6ed9632ad4ead0791f76 
> 
> 
> Diff: https://reviews.apache.org/r/62213/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 62214: Added JavaScript linter.

2017-09-11 Thread Armand Grillet


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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This linter runs when changes on a JavaScript file are being committed.
The linter used is ESLint with a configuration based on our current JS
code base. The linter and its dependencies (i.e. Node.js) are installed
in a virtual environment using Virtualenv and then Nodeenv.


Diffs
-

  src/.gitignore PRE-CREATION 
  src/webui/master/static/.eslintrc.js PRE-CREATION 
  src/webui/master/static/.gitignore PRE-CREATION 
  src/webui/master/static/bootstrap PRE-CREATION 
  src/webui/master/static/pip-requirements.txt PRE-CREATION 
  support/mesos-style.py cf37d9f4da4ab90b92f0136a1dcd5dd8bbae5785 


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


Testing
---

Following this commit, I have tried to commit a change on a JavaScript file and 
checked that ESLinter was correctly invoked.


Thanks,

Armand Grillet

Review Request 62212: Send TASK_STARTING from the built-in executors. [1/2]

2017-09-11 Thread Benno Evers


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

Review request for mesos, Andrei Budnik and Alexander Rukletsov.


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


Repository: mesos


Description
---

This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.


Diffs
-

  docs/high-availability-framework-guide.md 
73743aba31f9d0ca827d318e2ecb4752a91b1be0 
  src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79 
  src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529 
  src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082 


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


Testing
---


Thanks,

Benno Evers

Review Request 62213: Send TASK_STARTING from the built-in executors. [2/2]

2017-09-11 Thread Benno Evers


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

Review request for mesos, Andrei Budnik and Alexander Rukletsov.


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


Repository: mesos


Description
---

Fix unit tests for previous commit.


Diffs
-

  src/tests/api_tests.cpp 0902a66772d178505e83b81878b7878a45c7bb25 
  src/tests/check_tests.cpp cc7be317030d147c5617e784a50e42b0365b9df6 
  src/tests/command_executor_tests.cpp f706f55b5bfab824268498d95d775b216561cd66 
  src/tests/container_logger_tests.cpp 97e79792d3ea8023890ad2a705db47f2aeb419cf 
  src/tests/containerizer/environment_secret_isolator_tests.cpp 
38fef2d5f677f768a0533d1ac085b1197b3b764d 
  src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
0030cd1e1f73422bef806bbc0453134e3d7840d8 
  src/tests/default_executor_tests.cpp 186b8333c02ba3b9257e19437c6d689761085362 
  src/tests/disk_quota_tests.cpp 3bf0508238a228d86737d6cc899fa68e2046f2e2 
  src/tests/fault_tolerance_tests.cpp 5ac38a897fefb6f40d69ca6e27a6f23176e42d36 
  src/tests/health_check_tests.cpp 2c43241a5cc245a1797ea16c68c94638cec6803d 
  src/tests/master_tests.cpp 59fbad468ae671a8fd39457632262015989f26c2 
  src/tests/master_validation_tests.cpp 
710b25c1afdff4de7b2eb9b8a38f6856c373fb3c 
  src/tests/oversubscription_tests.cpp a4c4a6083a2cb55e2d69eba1719555a7a13ee8fb 
  src/tests/partition_tests.cpp 0886f4890ac3fec6f38146946892769a99c3e68f 
  src/tests/persistent_volume_endpoints_tests.cpp 
7a24bf48e9d84bf31ad85397ac3e1b4e566cbc2c 
  src/tests/persistent_volume_tests.cpp 
3e1d1fe468298186347b07b5882973c5a16231a3 
  src/tests/reconciliation_tests.cpp 728961874239c7e5ce8accbaeb2a1c86a73a4f0f 
  src/tests/reservation_endpoints_tests.cpp 
3278732cb130c73be0f20c0204eddaee05123ff9 
  src/tests/role_tests.cpp fc4c017dc59ab44a0ce0ea46f02eb6e1706ffb72 
  src/tests/scheduler_tests.cpp 21f8825c214bb0c331f66bfb9b12a354d6673a43 
  src/tests/slave_authorization_tests.cpp 
30eceae0920351cffc9c3393c6d08917c4041c1a 
  src/tests/slave_recovery_tests.cpp 0e46748be266809c413fb10fc447516c51504fce 
  src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 
  src/tests/teardown_tests.cpp 8834333bbcdb6a04a95a6ed9632ad4ead0791f76 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 62230: Avoid GC pruning events from blocking other processes.

Re: Review Request 62003: Added `network/ports` isolator nested container tests.

Re: Review Request 62047: Allowed look up latest executor directory by virtual path.

Re: Review Request 62047: Allowed look up latest executor directory by virtual path.

Re: Review Request 62040: Also log attached virtual path in agent.

Re: Review Request 62230: Avoid GC pruning events from blocking other processes.

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

Review Request 62230: Avoid GC pruning events from blocking other processes.

Re: Review Request 61473: Do not kill non partition aware tasks.

Re: Review Request 61473: Do not kill non partition aware tasks.

Re: Review Request 62214: Added JavaScript linter.

Re: Review Request 62003: Added `network/ports` isolator nested container tests.

Re: Review Request 60495: Added network ports isolator listen socket utilities.

Re: Review Request 62214: Added JavaScript linter.

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

Re: Review Request 62018: Updated logging::initialize to make flags optional.

Re: Review Request 62213: Send TASK_STARTING from the built-in executors. [2/2]

Review Request 62214: Added JavaScript linter.

Review Request 62212: Send TASK_STARTING from the built-in executors. [1/2]

Review Request 62213: Send TASK_STARTING from the built-in executors. [2/2]

20 matches

Site Navigation

Mail list logo

Footer information