Re: Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65617, 65618, 65619, 65620]

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

- Mesos Reviewbot


On Feb. 13, 2018, 12:58 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65620/
> ---
> 
> (Updated Feb. 13, 2018, 12:58 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, used .bat (cmd.exe) script to emulate hadoop
> (rather than a shell script).
> 
> Tested all `HadoopFetcherPlugin.*` tests, all succeed.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 
> 
> 
> Diff: https://reviews.apache.org/r/65620/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65570: Attached/detached volume directory for task which has volume specified.

[Qian Zhang]

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

(Updated Feb. 13, 2018, 2:47 p.m.)


Review request for mesos and Gilbert Song.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Attached/detached volume directory for task which has volume specified.


Diffs (updated)
-

  src/slave/slave.hpp 30151c4886e12e9183a971b86b854e28a8ca1b39 
  src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Benjamin Mahler]

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


Ship it!




Would be great to see some TODOs or a ticket to clean up the confusion around 
StartSlave not starting the slave when mock=true.

- Benjamin Mahler


On Feb. 12, 2018, 7:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 7:48 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Benjamin Mahler]

> On Feb. 10, 2018, 1:17 a.m., Benjamin Mahler wrote:
> > src/tests/mesos.cpp
> > Line 391 (original), 400 (patched)
> > 
> >
> > I realize this is a copy/paste, but do you know why we don't start() 
> > when mock is true?
> 
> Meng Zhu wrote:
> If we want to set up expect calls for the agent start code path, we need 
> to (1) create the mock slave instance; (2) setup expectations; (3) start the 
> slave.
> 
> Benjamin Mahler wrote:
> Ah that make sense, but it's confusing that StartSlave doesn't start the 
> slave if mock=false. Probably we should have only had something like 
> CreateSlave or CreateUnstartedSlave to avoid this confusion?

That makes sense, but it's confusing that StartSlave doesn't start a slave, 
isn't it? =/

Ideally we would have something more accurately named, like CreateSlave.


- Benjamin


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


On Feb. 12, 2018, 7:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 7:48 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65593: Added tests to check executor that failed to launch is removed.

[Meng Zhu]

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

(Updated Feb. 12, 2018, 10:26 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Added one more test for task authorization failure.


Summary (updated)
-

Added tests to check executor that failed to launch is removed.


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


Repository: mesos


Description (updated)
---

Theses tests ensure that agent sends ExitedExecutorMessage when the
task group fails to launch due to unschedule GC failure and when
the task fails to launch due to task authorization failure.
So that master's executor bookkeeping entry is removed.


Diffs (updated)
-

  src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 65626: Added mock method for `__run()` in mock slave.

[Meng Zhu]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added mock method for `__run()` in mock slave.


Diffs
-

  src/slave/slave.hpp 30151c4886e12e9183a971b86b854e28a8ca1b39 
  src/tests/mock_slave.hpp 942ead57fc67bdd2a268c67575952349838dc280 
  src/tests/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Benjamin Mahler]

> On Feb. 10, 2018, 1:17 a.m., Benjamin Mahler wrote:
> > src/tests/mesos.cpp
> > Line 391 (original), 400 (patched)
> > 
> >
> > I realize this is a copy/paste, but do you know why we don't start() 
> > when mock is true?
> 
> Meng Zhu wrote:
> If we want to set up expect calls for the agent start code path, we need 
> to (1) create the mock slave instance; (2) setup expectations; (3) start the 
> slave.

Ah that make sense, but it's confusing that StartSlave doesn't start the slave 
if mock=false. Probably we should have only had something like CreateSlave or 
CreateUnstartedSlave to avoid this confusion?


- Benjamin


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


On Feb. 12, 2018, 7:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 7:48 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65613: Fixed a bug where MockSlave `id` is not properly initialized.

[Benjamin Mahler]

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


Ship it!




Thanks!

- Benjamin Mahler


On Feb. 12, 2018, 7:32 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65613/
> ---
> 
> (Updated Feb. 12, 2018, 7:32 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `MockSlave` did not initialize the id field using the input
> argument. Also it is necessary to call its virtual base class
> `ProcessBase` constructor explicitly to avoid argument `id` being
> lost.
> 
> 
> Diffs
> -
> 
>   src/tests/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 
> 
> 
> Diff: https://reviews.apache.org/r/65613/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65624 was successfully built and tested.

Reviews applied: `['65403', '65467', '65574', '65469', '65624']`

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

- Mesos Reviewbot Windows


On Feb. 13, 2018, 4:41 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 13, 2018, 4:41 a.m.)
> 
> 
> Review request for mesos, Akash Gupta and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65616: Removed outdated executor-wide launched flag from the default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65548, 65549, 65550, 65551, 65552, 65556, 65616]

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

- Mesos Reviewbot


On Feb. 12, 2018, 11:41 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65616/
> ---
> 
> (Updated Feb. 12, 2018, 11:41 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed outdated executor-wide launched flag from the default executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65616/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65518 was successfully built and tested.

Reviews applied: `['65518']`

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

- Mesos Reviewbot Windows


On Feb. 13, 2018, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 13, 2018, 3:20 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8488
> https://issues.apache.org/jira/browse/MESOS-8488
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to a Docker issue (https://github.com/moby/moby/issues/33820),
> Docker daemon can fail to catch a container exit, i.e., the container
> process has already exited but the command `docker ps` shows the
> container still running, this will lead to the "docker run" command
> that we execute in Docker executor never returning, and it will also
> cause the `docker stop` command takes no effect, i.e., it will return
> without error but `docker ps` shows the container still running, so
> the task will stuck in `TASK_KILLING` state.
> 
> To workaround this Docker issue, in this patch we made Docker executor
> reaps the container process directly so Docker executor will be notified
> once the container process exits.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65518/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 65624: Added fetcher tests for `stderr` output.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta and Joseph Wu.


Repository: mesos


Description
---

The fetcher is supposed to log its `stderr` output to a redirected file
called `stderr` in the given sandbox directory. There previously existed
a bug on Windows due to incorrect handle inheritance where this
redirection failed silently, leaving the log empty. These unit tests
assert that the correct content is logged to the `stderr` file for both
a success and failure fetch scenario.


Diffs
-

  src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 


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


Testing
---

Windows (Linux pending):
```
[ RUN  ] FetcherTest.LogSuccessToStderr
[   OK ] FetcherTest.LogSuccessToStderr (197 ms)
[ RUN  ] FetcherTest.LogFailureToStderr
[   OK ] FetcherTest.LogFailureToStderr (320 ms)
```


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

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

(Updated Feb. 13, 2018, 11:20 a.m.)


Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod 
Kone.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

Due to a Docker issue (https://github.com/moby/moby/issues/33820),
Docker daemon can fail to catch a container exit, i.e., the container
process has already exited but the command `docker ps` shows the
container still running, this will lead to the "docker run" command
that we execute in Docker executor never returning, and it will also
cause the `docker stop` command takes no effect, i.e., it will return
without error but `docker ps` shows the container still running, so
the task will stuck in `TASK_KILLING` state.

To workaround this Docker issue, in this patch we made Docker executor
reaps the container process directly so Docker executor will be notified
once the container process exits.


Diffs (updated)
-

  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 65395: Refactored health checks to cleanly separate each different check.

[Joseph Wu]

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



I like what I see.  Just a few little things to discuss.


src/checks/checker_process.cpp
Lines 210-211 (patched)


For future reference, you can usually replace this with a `UNREACHABLE()`.



src/checks/checker_process.cpp
Line 217 (original), 232-235 (patched)


Why not move the body of the `checkInfoToCheck` helper into the 
constructor?  It doesn't see like it is used anywhere else.

It is better to justify a `LOG(FATAL)` like this:
```
case CheckInfo::UNKNOWN: {
  // TODO(josephw): Add validation at the agent or master level. 
  // Without validation, this is considered a mis-configuration of the task 
(user error).
  LOG(FATAL) << "Received UNKNOWN check type";
}
```

You may also be able to remove the `Try<>` portion of the `check` object if 
you construct it here.



src/checks/checker_process.cpp
Lines 283-284 (patched)


Looks like the last `+` to the left of `TCP` is not aligned :D



src/checks/checker_process.cpp
Lines 297-298 (patched)


I believe it won't be too difficult to get nested containers working on 
Windows (no images, just nested job objects).

If we can enable the tests under `agent_container_api_tests.cpp`, that 
should give sufficient confidence that nested containers work.  There are 
separate tests for health checks that can be enabled later (like ` 
HealthCheckTest.DefaultExecutorCommandHealthCheck`).


- Joseph Wu


On Feb. 8, 2018, 9:49 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> ---
> 
> (Updated Feb. 8, 2018, 9:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored health check code to separate the logic for each check
> type and runtime (Plain/Docker/Nested). Now the matrix of different
> possiblilites is cleanly enumerated, making it easier to add
> future health checks, such as the ones for Windows.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 164ecc7ba51f75db6ec581aa4b135526c304dd00 
> 
> 
> Diff: https://reviews.apache.org/r/65395/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65620 was successfully built and tested.

Reviews applied: `['65617', '65618', '65619', '65620']`

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

- Mesos Reviewbot Windows


On Feb. 13, 2018, 12:58 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65620/
> ---
> 
> (Updated Feb. 13, 2018, 12:58 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, used .bat (cmd.exe) script to emulate hadoop
> (rather than a shell script).
> 
> Tested all `HadoopFetcherPlugin.*` tests, all succeed.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 
> 
> 
> Diff: https://reviews.apache.org/r/65620/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 64604: Windows: Updated heath-checks.md with Windows implementation.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [64604, 64387]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On Jan. 5, 2018, 12:32 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64604/
> ---
> 
> (Updated Jan. 5, 2018, 12:32 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, and Michael Park.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated health-checks.md with details of the Windows health check
> implementations for the mesos and docker executors.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md 119d149f29f2f2d3178da6ef63a7ce97a4dbc952 
> 
> 
> Diff: https://reviews.apache.org/r/64604/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65570: Attached/detached volume directory for task which has volume specified.

[Vinod Kone]

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



Can you add tests for this in a subsequent review please?


src/slave/slave.hpp
Line 434 (original), 442 (patched)


s/at it to make it can be/to it so that it can be/



src/slave/slave.hpp
Line 435 (original), 443 (patched)


s/, so/. So/



src/slave/slave.cpp
Line 1043 (original), 1044 (patched)


can you fix the naming to be consistent with below comments?



src/slave/slave.cpp
Lines 1075 (patched)


`containerInfo`



src/slave/slave.cpp
Lines 1078-1087 (patched)


kill this. see below.



src/slave/slave.cpp
Lines 1089-1091 (patched)


kill this.



src/slave/slave.cpp
Lines 1109- (patched)


I don't think we need this check. We want to show non PV directories too in 
the UI.



src/slave/slave.cpp
Lines 1113 (patched)


executorRunPath



src/slave/slave.cpp
Lines 1120 (patched)


s/executorVolumePath/executorDirectoryPath/



src/slave/slave.cpp
Lines 1131 (patched)


taskDirectoryPath



src/slave/slave.cpp
Lines 1157-1167 (patched)


kill this.



src/slave/slave.cpp
Lines 1196-1198 (patched)


kill this.



src/slave/slave.cpp
Lines 1219-1221 (patched)


kill this.



src/slave/slave.cpp
Lines 1231 (patched)


taskDirectoryPath


- Vinod Kone


On Feb. 10, 2018, 2:59 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65570/
> ---
> 
> (Updated Feb. 10, 2018, 2:59 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8565
> https://issues.apache.org/jira/browse/MESOS-8565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Attached/detached volume directory for task which has volume specified.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 30151c4886e12e9183a971b86b854e28a8ca1b39 
>   src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 
> 
> 
> Diff: https://reviews.apache.org/r/65570/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65616: Removed outdated executor-wide launched flag from the default executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65616 was successfully built and tested.

Reviews applied: `['65548', '65549', '65550', '65551', '65552', '65556', 
'65616']`

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

- Mesos Reviewbot Windows


On Feb. 12, 2018, 3:41 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65616/
> ---
> 
> (Updated Feb. 12, 2018, 3:41 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed outdated executor-wide launched flag from the default executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65616/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65613, 65497]

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

- Mesos Reviewbot


On Feb. 12, 2018, 11:48 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 11:48 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 4:59 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65618/
> ---
> 
> (Updated Feb. 12, 2018, 4:59 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled both `curl` and `hadoop` fetcher plugins on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 
>   src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 
> 
> 
> Diff: https://reviews.apache.org/r/65618/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 12, 2018, 3:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 12, 2018, 3:46 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8488
> https://issues.apache.org/jira/browse/MESOS-8488
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to a Docker issue (https://github.com/moby/moby/issues/33820),
> Docker daemon will fail to catch a container exit, i.e., the container
> process has already exited but the command `docker ps` shows the
> container still running, this will lead to the "docker run" command
> that we execute in Docker executor never returns, and it will also
> cause the `docker stop` command takes no effect, i.e., it will return
> without error but `docker ps` shows the container still running, so
> the task will stuck in `TASK_KILLING` state.
> 
> To workaround this Docker issue, in this patch we made Docker executor
> reaps the container process directly so Docker executor will be notified
> once the container process exits.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65518/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Vinod Kone]

> On Feb. 10, 2018, 12:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Lines 280 (patched)
> > 
> >
> > when can this be None()?
> 
> Qian Zhang wrote:
> According to this comment 
> https://github.com/apache/mesos/blob/1.5.0/src/docker/docker.hpp#L104:L106, 
> it will be `None()` when the container is not running.
> 
> But I am not sure if it will be `None()` when the container is not 
> running AND the Docker issue (https://github.com/moby/moby/issues/33820) 
> occurs. I mean if we launch a Docker container which exits immediately (e.g., 
> execute the command `exit 0`), and due to that Docker issue Docker daemon 
> does not catch the container's exit, will `container.pid` be `None()` or not? 
> If it is not `None()` in this case, then we will reap the process in this 
> lambda which is good, but if it is `None()`, then we will miss to reap the 
> process which is not correct. My suspect is it will not be `None` in this 
> case, but just to be safe, let's also do the below in the case that 
> `container.pid` is `None()`, how do you think?
> ```
> delay(
> Seconds(3),
> self(),
> &Self::reapedContainer,
> container.pid.get());
> ```

yes, lets do this when pid is None(). also, consider returning Nothing() if 
`terminated` is set.


> On Feb. 10, 2018, 12:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Lines 288 (patched)
> > 
> >
> > Add a LOG line here?
> 
> Qian Zhang wrote:
> If we add a LOG here, then we may need to add a LOG into `reaped` too? 
> They are the two methods to catch the exit status of the container.

No need for a log here if you are already doing it in `reapedContainer`


- Vinod


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


On Feb. 12, 2018, 3:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 12, 2018, 3:46 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8488
> https://issues.apache.org/jira/browse/MESOS-8488
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to a Docker issue (https://github.com/moby/moby/issues/33820),
> Docker daemon will fail to catch a container exit, i.e., the container
> process has already exited but the command `docker ps` shows the
> container still running, this will lead to the "docker run" command
> that we execute in Docker executor never returns, and it will also
> cause the `docker stop` command takes no effect, i.e., it will return
> without error but `docker ps` shows the container still running, so
> the task will stuck in `TASK_KILLING` state.
> 
> To workaround this Docker issue, in this patch we made Docker executor
> reaps the container process directly so Docker executor will be notified
> once the container process exits.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65518/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.

[Jeff Coffler]

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

(Updated Feb. 13, 2018, 12:59 a.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Enabled both `curl` and `hadoop` fetcher plugins on Windows platform.


Diffs (updated)
-

  src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 
  src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 


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

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


Testing
---


Thanks,

Jeff Coffler

Re: Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.

[Jeff Coffler]

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

(Updated Feb. 13, 2018, 12:58 a.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description (updated)
---

Fix Windows-specific issues (absolute path, subprocess invocation)
to allow Hadoop Fetcher Plugin to run properly.


Diffs (updated)
-

  src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 


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

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


Testing
---

Full test pass on both Linux and Windows.

Additionally, on Windows platform:

```
[==] Running 3 tests from 1 test case.
[--] Global test environment set-up.
[--] 3 tests from HadoopFetcherPluginTest
[ RUN  ] HadoopFetcherPluginTest.FetchExistingFile

C:\Users\jeffcof\AppData\Local\Temp\UrtxvI>if "version" == "version" (exit 0 )
[   OK ] HadoopFetcherPluginTest.FetchExistingFile (235 ms)
[ RUN  ] HadoopFetcherPluginTest.FetchNonExistingFile

C:\Users\jeffcof\AppData\Local\Temp\YJVcTA>if "version" == "version" (exit 0 )
[   OK ] HadoopFetcherPluginTest.FetchNonExistingFile (217 ms)
[ RUN  ] HadoopFetcherPluginTest.InvokeFetchByName

C:\Users\jeffcof\AppData\Local\Temp\wsQjH1>if "version" == "version" (exit 0 )
[   OK ] HadoopFetcherPluginTest.InvokeFetchByName (217 ms)
[--] 3 tests from HadoopFetcherPluginTest (674 ms total)

[--] Global test environment tear-down
[==] 3 tests from 1 test case ran. (12790 ms total)
[  PASSED  ] 3 tests.
```


Thanks,

Jeff Coffler

Re: Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.

[Jeff Coffler]

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

(Updated Feb. 13, 2018, 12:58 a.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

On Windows, used .bat (cmd.exe) script to emulate hadoop
(rather than a shell script).

Tested all `HadoopFetcherPlugin.*` tests, all succeed.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 


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

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


Testing
---


Thanks,

Jeff Coffler

Re: Review Request 65619: Windows: Modified build system to build hadoop plugins.

[Jeff Coffler]

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

(Updated Feb. 13, 2018, 12:57 a.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description (updated)
---

Windows: Modified build system to build hadoop plugins.


Diffs (updated)
-

  src/CMakeLists.txt 21fb47e29dd0b19681690b8de5261c68b574a7c8 


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

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


Testing
---


Thanks,

Jeff Coffler

Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.

[Jeff Coffler]

> On Feb. 13, 2018, 12:08 a.m., Andrew Schwartzmeyer wrote:
> > I looked at MESOS-5473, and I had previously closed it because it didn't 
> > make sense.
> > 
> > Would you open a new issue: "Enable Docker fetcher plugin on Windows" and 
> > update the comments here?

Done, MESOS-8570: Enable Docker fetcher plugin on Windows.


- Jeff


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


On Feb. 12, 2018, 11:31 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65618/
> ---
> 
> (Updated Feb. 12, 2018, 11:31 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled both `curl` and `hadoop` fetcher plugins on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 
>   src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 
> 
> 
> Diff: https://reviews.apache.org/r/65618/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65394: Added separate structs for health check runtime and check types.

[Andrew Schwartzmeyer]

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




src/checks/checks_runtime.hpp
Lines 1-15 (patched)


Both these files probably need to be added to Autotools... when was the 
last time you tried a `../configure && make check`? I'd expect it to break with 
new headers.


- Andrew Schwartzmeyer


On Feb. 12, 2018, 2:50 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65394/
> ---
> 
> (Updated Feb. 12, 2018, 2:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is in preparation of another patch that will refactor the
> health check code using these types.
> 
> 
> Diffs
> -
> 
>   src/checks/checks_runtime.hpp PRE-CREATION 
>   src/checks/checks_types.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65394/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64604: Windows: Updated heath-checks.md with Windows implementation.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Jan. 4, 2018, 4:32 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64604/
> ---
> 
> (Updated Jan. 4, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, and Michael Park.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated health-checks.md with details of the Windows health check
> implementations for the mesos and docker executors.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md 119d149f29f2f2d3178da6ef63a7ce97a4dbc952 
> 
> 
> Diff: https://reviews.apache.org/r/64604/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64387: Windows: Ported docker health check tests.

[Andrew Schwartzmeyer]

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



This patch looks like it's still pending updates (i.e. it has open issues).

- Andrew Schwartzmeyer


On Feb. 12, 2018, 4:24 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64387/
> ---
> 
> (Updated Feb. 12, 2018, 4:24 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `HealthCheckTest.ROOT_DOCKER_*` and
> `DockerContainerizerHealthCheckTest.*` tests now work on Windows.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_common.hpp PRE-CREATION 
>   src/tests/environment.cpp 931e647c9531dc27c31a02b9e8aca15ef441a5b6 
>   src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
> 
> 
> Diff: https://reviews.apache.org/r/64387/diff/11/
> 
> 
> Testing
> ---
> 
> Windows Server:
> [==] Running 5 tests from 2 test cases.
> [--] Global test environment set-up.
> [--] 2 tests from HealthCheckTest
> [ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask
> [   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask (21263 ms)
> [ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> [   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange (23512 ms)
> [--] 2 tests from HealthCheckTest (44835 ms total)
> 
> [--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
>  (28487 ms)
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0
>  (26447 ms)
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0
>  (26264 ms)
> [--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest 
> (81268 ms total)
> 
> [--] Global test environment tear-down
> [==] 5 tests from 2 test cases ran. (126559 ms total)
> [  PASSED  ] 5 tests
> 
> Rest of tests pass.
> 
> Windows Client (Disabled network health checks):
> Proof that network health checks are skipped on Windows Client.
> C:\Program Files\Docker\Docker\Resources\bin\docker.exe: Error response from 
> daemon: sharing of hyperv containers network is not supported.
> 356b087e7fa640f83fe27ebeb3396bfc7b2bbebd917aeaec0508b887b41d31f4
> -
> We cannot run any Docker health checks tests because:
> Running in another container's namespace is not supported on this version of 
> Windows.
> 
> Rest rests pass.
> 
> Linux:
> make check passes
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65620 was successfully built and tested.

Reviews applied: `['65617', '65618', '65619', '65620']`

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

- Mesos Reviewbot Windows


On Feb. 12, 2018, 11:34 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65620/
> ---
> 
> (Updated Feb. 12, 2018, 11:34 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, used .bat (cmd.exe) script to emulate hadoop
> (rather than a shell script).
> 
> Tested all `HadoopFetcherPlugin.*` tests, all succeed.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 
> 
> 
> Diff: https://reviews.apache.org/r/65620/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65615: Added docker constants and common functions file.

[Andrew Schwartzmeyer]

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




src/tests/containerizer/docker_common.hpp
Lines 126 (patched)


`status()->get()`



src/tests/containerizer/docker_common.hpp
Lines 163 (patched)


Ditto. Avoid chained .get()s.


- Andrew Schwartzmeyer


On Feb. 12, 2018, 4:22 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65615/
> ---
> 
> (Updated Feb. 12, 2018, 4:22 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is in preparation of porting docker tests to Windows. There
> were a few hardcoded values that were used throughout the codebase,
> such as the `alpine` image, that are defined as a constant in a single
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_common.hpp PRE-CREATION 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> d1e657050d623ad0412208b3aa3e3101e3654e99 
>   src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
> 
> 
> Diff: https://reviews.apache.org/r/65615/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64604: Windows: Updated heath-checks.md with Windows implementation.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['65393', '65394', '65395', '65396', '65419', '65127', 
'64387', '64604']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

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

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (36 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (69 ms 
total)

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

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

[--] Global test environment tear-down
[==] 900 tests from 89 test cases ran. (410604 ms total)
[  PASSED  ] 900 tests.
[  FAILED  ] 0 tests, listed below:

 0 FAILED TESTS

  YOU HAVE 209 DISABLED TESTS

```

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

```
I0213 00:39:56.722865  5588 hierarchical.cpp:405] Deactivated framework 
8c6264fb-5624-467c-adf9-47b9e8abc0ef-
I0213 0I0213 00:39:56.056852 10396 exec.cpp:162] Version: 1.6.0
I0213 00:39:56.079854  8272 exec.cpp:236] Executor registered on agent 
8c6264fb-5624-467c-adf9-47b9e8abc0ef-S0
I0213 00:39:56.082854  6064 executor.cpp:176] Received SUBSCRIBED event
I0213 00:39:56.087848  6064 executor.cpp:180] Subscribed executor on 
build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0213 00:39:56.087848  6064 executor.cpp:176] Received LAUNCH event
I0213 00:39:56.092849  6064 executor.cpp:648] Starting task 
cf82aec3-57aa-4e19-a214-cf947ad7ab84
I0213 00:39:56.164849  6064 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0213 00:39:56.696864  6064 executor.cpp:661] Forked command at 9096
I0213 00:39:56.724865  6376 exec.cpp:445] Executor asked to shutdown
I0213 00:39:56.725864  7236 executor.cpp:176] Received SHUTDOWN event
I0213 00:39:56.725864  7236 executor.cpp:758] Shutting down
I0213 00:39:56.725864  7236 executor.cpp:873] Sending SIGTERM to process tree 
at pid 90:39:56.722865 10044 slave.cpp:3501] Shutting down framework 
8c6264fb-5624-467c-adf9-47b9e8abc0ef-
I0213 00:39:56.722865 10044 slave.cpp:6214] Shutting down executor 
'cf82aec3-57aa-4e19-a214-cf947ad7ab84' of framework 
8c6264fb-5624-467c-adf9-47b9e8abc0ef- at executor(1)@10.3.1.11:53994
I0213 00:39:56.723865 10044 slave.cpp:931] Agent terminating
W0213 00:39:56.724865 10044 slave.cpp:3497] Ignoring shutdown framework 
8c6264fb-5624-467c-adf9-47b9e8abc0ef- because it is terminating
I0213 00:39:56.726866  1668 master.cpp:10304] Removing task 
cf82aec3-57aa-4e19-a214-cf947ad7ab84 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 8c6264fb-5624-467c-adf9-47b9e8abc0ef- on 
agent 8c6264fb-5624-467c-adf9-47b9e8abc0ef-S0 at slave(406)@10.3.1.11:53973 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0213 00:39:56.726866 10044 containerizer.cpp:2338] Destroying container 
dbef38b0-e08b-4ae3-a290-6dca376dbcbc in RUNNING state
I0213 00:39:56.727865 10044 containerizer.cpp:2952] Transitioning the state of 
container dbef38b0-e08b-4ae3-a290-6dca376dbcbc from RUNNING to DESTROYING
I0213 00:39:56.728865 10044 launcher.cpp:156] Asked to destroy container 
dbef38b0-e08b-4ae3-a290-6dca376dbcbc
I0213 00:39:56.728865  1668 master.cpp:1307] Agent 
8c6264fb-5624-467c-adf9-47b9e8abc0ef-S0 at slave(406)@10.3.1.11:53973 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0213 00:39:56.728865  1668 master.cpp:3277] Disconnecting agent 
8c6264fb-5624-467c-adf9-47b9e8abc0ef-S0 at slave(406)@10.3.1.11:53973 
(build-srv-03.zq4gs31qjdiunm1ryi1

Re: Review Request 65615: Added docker constants and common functions file.

[Andrew Schwartzmeyer]

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




src/tests/containerizer/docker_common.hpp
Lines 1-18 (patched)


Did this header get added to Autotools where necessary?


- Andrew Schwartzmeyer


On Feb. 12, 2018, 4:22 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65615/
> ---
> 
> (Updated Feb. 12, 2018, 4:22 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is in preparation of porting docker tests to Windows. There
> were a few hardcoded values that were used throughout the codebase,
> such as the `alpine` image, that are defined as a constant in a single
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_common.hpp PRE-CREATION 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> d1e657050d623ad0412208b3aa3e3101e3654e99 
>   src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
> 
> 
> Diff: https://reviews.apache.org/r/65615/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:16 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 12, 2018, 3:16 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65419: Fixed quoting issues in docker executor command checks.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 8, 2018, 9:50 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> ---
> 
> (Updated Feb. 8, 2018, 9:50 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, and Joseph Wu.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker executor was wrapping the docker exec command around with
> `sh -c ""`, which was causing quoting issues, especially on Windows.
> Now, the comamnd health check simply runs `docker exec` without any
> wrapping.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65419/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65396: Moved docker command check code inside health check library.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 8, 2018, 9:50 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65396/
> ---
> 
> (Updated Feb. 8, 2018, 9:50 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now with the health check library refactor, the wrapping of `docker
> exec` for docker command health checks can be easily moved inside
> the library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65396/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65395: Refactored health checks to cleanly separate each different check.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 8, 2018, 9:49 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> ---
> 
> (Updated Feb. 8, 2018, 9:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored health check code to separate the logic for each check
> type and runtime (Plain/Docker/Nested). Now the matrix of different
> possiblilites is cleanly enumerated, making it easier to add
> future health checks, such as the ones for Windows.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 164ecc7ba51f75db6ec581aa4b135526c304dd00 
> 
> 
> Diff: https://reviews.apache.org/r/65395/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65394: Added separate structs for health check runtime and check types.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 2:50 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65394/
> ---
> 
> (Updated Feb. 12, 2018, 2:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is in preparation of another patch that will refactor the
> health check code using these types.
> 
> 
> Diffs
> -
> 
>   src/checks/checks_runtime.hpp PRE-CREATION 
>   src/checks/checks_types.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65394/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65393: Fixed docker command health check to use the right docker socket.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 2:50 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65393/
> ---
> 
> (Updated Feb. 12, 2018, 2:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The original command health check was calling `docker exec` instead
> of `docker -H  exec`, so it was ignoring the socket
> value passed to the docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65393/diff/2/
> 
> 
> Testing
> ---
> 
> ran mesos-tests --docker flag.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64387: Windows: Ported docker health check tests.

[Akash Gupta]

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

(Updated Feb. 13, 2018, 12:24 a.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, Joseph Wu, and Michael Park.


Changes
---

Rebased + feedback.


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


Repository: mesos


Description
---

The `HealthCheckTest.ROOT_DOCKER_*` and
`DockerContainerizerHealthCheckTest.*` tests now work on Windows.


Diffs (updated)
-

  src/tests/containerizer/docker_common.hpp PRE-CREATION 
  src/tests/environment.cpp 931e647c9531dc27c31a02b9e8aca15ef441a5b6 
  src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 


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

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


Testing
---

Windows Server:
[==] Running 5 tests from 2 test cases.
[--] Global test environment set-up.
[--] 2 tests from HealthCheckTest
[ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask
[   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask (21263 ms)
[ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
[   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange (23512 ms)
[--] 2 tests from HealthCheckTest (44835 ms total)

[--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest
[ RUN  ] 
NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
[   OK ] 
NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
 (28487 ms)
[ RUN  ] 
NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0
[   OK ] 
NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0
 (26447 ms)
[ RUN  ] 
NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0
[   OK ] 
NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0
 (26264 ms)
[--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest 
(81268 ms total)

[--] Global test environment tear-down
[==] 5 tests from 2 test cases ran. (126559 ms total)
[  PASSED  ] 5 tests

Rest of tests pass.

Windows Client (Disabled network health checks):
Proof that network health checks are skipped on Windows Client.
C:\Program Files\Docker\Docker\Resources\bin\docker.exe: Error response from 
daemon: sharing of hyperv containers network is not supported.
356b087e7fa640f83fe27ebeb3396bfc7b2bbebd917aeaec0508b887b41d31f4
-
We cannot run any Docker health checks tests because:
Running in another container's namespace is not supported on this version of 
Windows.

Rest rests pass.

Linux:
make check passes


Thanks,

Akash Gupta

Review Request 65615: Added docker constants and common functions file.

[Akash Gupta]

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

Review request for mesos.


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


Repository: mesos


Description
---

This patch is in preparation of porting docker tests to Windows. There
were a few hardcoded values that were used throughout the codebase,
such as the `alpine` image, that are defined as a constant in a single
file.


Diffs
-

  src/tests/containerizer/docker_common.hpp PRE-CREATION 
  src/tests/containerizer/docker_containerizer_tests.cpp 
d1e657050d623ad0412208b3aa3e3101e3654e99 
  src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
  src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 


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


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65449: Fixed an bug where executor info lingers on master if failed to launch.

[Meng Zhu]

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

(Updated Feb. 12, 2018, 4:11 p.m.)


Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.


Changes
---

`"Executor" +  executor->state` was complained on Mac, changed it to plain 
string.


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


Repository: mesos


Description
---

Master relies on `ExitedExecutorMessage` from the agent to recycle
executor entry. However, this message won't be sent if the executor
never actually launched (due to transient error), leaving executor
info on the master lingering and resource claimed.
See MESOS-1720.

This patch fixes this issue by sending the `ExitedExecutorMessage`
from the agent if the executor is never launched.


Diffs (updated)
-

  src/slave/slave.hpp 30151c4886e12e9183a971b86b854e28a8ca1b39 
  src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 
  src/tests/mock_slave.hpp 942ead57fc67bdd2a268c67575952349838dc280 
  src/tests/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 
  src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 


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

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


Testing
---

make check
Dedicated test in #65448


Thanks,

Meng Zhu

Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.

[Andrew Schwartzmeyer]

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



I looked at MESOS-5473, and I had previously closed it because it didn't make 
sense.

Would you open a new issue: "Enable Docker fetcher plugin on Windows" and 
update the comments here?

- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:31 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65618/
> ---
> 
> (Updated Feb. 12, 2018, 3:31 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled both `curl` and `hadoop` fetcher plugins on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 
>   src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 
> 
> 
> Diff: https://reviews.apache.org/r/65618/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:34 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65620/
> ---
> 
> (Updated Feb. 12, 2018, 3:34 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, used .bat (cmd.exe) script to emulate hadoop
> (rather than a shell script).
> 
> Tested all `HadoopFetcherPlugin.*` tests, all succeed.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 
> 
> 
> Diff: https://reviews.apache.org/r/65620/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65619: Windows: Modified build system to build hadoop plugins.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:33 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65619/
> ---
> 
> (Updated Feb. 12, 2018, 3:33 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified cmake build system to build hadoop plugins.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 21fb47e29dd0b19681690b8de5261c68b574a7c8 
> 
> 
> Diff: https://reviews.apache.org/r/65619/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.

[Andrew Schwartzmeyer]

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




src/uri/fetcher.hpp
Lines 40-49 (original), 40-50 (patched)


```
class Flags :

  public virtual CopyFetcherPlugin::Flags,
  public virtual CurlFetcherPlugin::Flags,

#ifdef __WINDOWS__

  public virtual HadoopFetcherPlugin::Flags {};

#else

  // TODO(coffler): Add support for Docker plugins. See MESOS-5473.

  public virtual HadoopFetcherPlugin::Flags,

  public virtual DockerFetcherPlugin::Flags {};

#endif // __WINDOWS__
```



src/uri/fetcher.cpp
Lines 56 (patched)


s/dpravat/coffler



src/uri/fetcher.cpp
Lines 56 (patched)


s/dpravat/coffler


- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:31 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65618/
> ---
> 
> (Updated Feb. 12, 2018, 3:31 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled both `curl` and `hadoop` fetcher plugins on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 
>   src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 
> 
> 
> Diff: https://reviews.apache.org/r/65618/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:35 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65617/
> ---
> 
> (Updated Feb. 12, 2018, 3:35 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Windows-specific issues (absolute path, subprocess invocation)
> to allow Hadoop Fetcher Plugin to run properly.
> 
> 
> Diffs
> -
> 
>   src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 
> 
> 
> Diff: https://reviews.apache.org/r/65617/diff/1/
> 
> 
> Testing
> ---
> 
> Full test pass on both Linux and Windows.
> 
> Additionally, on Windows platform:
> 
> ```
> [==] Running 3 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 3 tests from HadoopFetcherPluginTest
> [ RUN  ] HadoopFetcherPluginTest.FetchExistingFile
> 
> C:\Users\jeffcof\AppData\Local\Temp\UrtxvI>if "version" == "version" (exit 0 )
> [   OK ] HadoopFetcherPluginTest.FetchExistingFile (235 ms)
> [ RUN  ] HadoopFetcherPluginTest.FetchNonExistingFile
> 
> C:\Users\jeffcof\AppData\Local\Temp\YJVcTA>if "version" == "version" (exit 0 )
> [   OK ] HadoopFetcherPluginTest.FetchNonExistingFile (217 ms)
> [ RUN  ] HadoopFetcherPluginTest.InvokeFetchByName
> 
> C:\Users\jeffcof\AppData\Local\Temp\wsQjH1>if "version" == "version" (exit 0 )
> [   OK ] HadoopFetcherPluginTest.InvokeFetchByName (217 ms)
> [--] 3 tests from HadoopFetcherPluginTest (674 ms total)
> 
> [--] Global test environment tear-down
> [==] 3 tests from 1 test case ran. (12790 ms total)
> [  PASSED  ] 3 tests.
> ```
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65616: Removed outdated executor-wide launched flag from the default executor.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

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

Relevant logs:

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

```
error: patch failed: src/launcher/default_executor.cpp:540
error: src/launcher/default_executor.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 12, 2018, 11:41 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65616/
> ---
> 
> (Updated Feb. 12, 2018, 11:41 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed outdated executor-wide launched flag from the default executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65616/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65551: Stopped shutting down the whole default executor on task launch failure.

[Gaston Kleiman]

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

(Updated Feb. 12, 2018, 3:41 p.m.)


Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod Kone.


Changes
---

Addressed feedback.

Note: lots of changes to `waited()`.


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


Repository: mesos


Description
---

The default executor would be completely shutdown on a
`LAUNCH_NESTED_CONTAINER` failure.

This patch makes it kill the affected task group instead of shutting
down and killing all task groups.


Diffs (updated)
-

  src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 


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

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


Testing
---

`sudo make check` on GNU/Linux

Regression test on https://reviews.apache.org/r/65552/


Thanks,

Gaston Kleiman

Re: Review Request 65551: Stopped shutting down the whole default executor on task launch failure.

[Gaston Kleiman]

> On Feb. 12, 2018, 12:37 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp
> > Line 786 (original), 788 (patched)
> > 
> >
> > If a launch for nested container fails, don't we get `NotFound`?

This didn't occur while doing manual testing, and isn't triggered by the test 
added in https://reviews.apache.org/r/65552.

However I took a look at the Mesos containerizer's code, and there are cases in 
which a 404 NOT FOUND could be returned, so I updated `waited()` to handle this.


- Gaston


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


On Feb. 9, 2018, 10:35 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65551/
> ---
> 
> (Updated Feb. 9, 2018, 10:35 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would be completely shutdown on a
> `LAUNCH_NESTED_CONTAINER` failure.
> 
> This patch makes it kill the affected task group instead of shutting
> down and killing all task groups.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65551/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> Regression test on https://reviews.apache.org/r/65552/
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65551: Stopped shutting down the whole default executor on task launch failure.

[Gaston Kleiman]

> On Feb. 12, 2018, 11:57 a.m., Joseph Wu wrote:
> > src/launcher/default_executor.cpp
> > Line 1479 (original), 1492 (patched)
> > 
> >
> > This booleans seems like a remnant of the time when the default 
> > executor would only launch a single task group.  The value is initialized 
> > to false, and is flipped to true upon launching the first TaskGroup.
> > 
> > It is `CHECK`'d in `__launchGroup(...)` (seems redundant, as that is a 
> > continuation of `launchGroup` where `launched = true;` is set); in `wait()` 
> > (similarly redundant, but has more entrypoints); and in `shutdown()`.
> > 
> > You can consider removing the bool in a separate patch.
> 
> Vinod Kone wrote:
> I made the same comment in the previous review :)

Boolean removed in https://reviews.apache.org/r/65616


- Gaston


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


On Feb. 9, 2018, 10:35 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65551/
> ---
> 
> (Updated Feb. 9, 2018, 10:35 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would be completely shutdown on a
> `LAUNCH_NESTED_CONTAINER` failure.
> 
> This patch makes it kill the affected task group instead of shutting
> down and killing all task groups.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65551/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> Regression test on https://reviews.apache.org/r/65552/
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Review Request 65616: Removed outdated executor-wide launched flag from the default executor.

[Gaston Kleiman]

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

Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod Kone.


Repository: mesos


Description
---

Removed outdated executor-wide launched flag from the default executor.


Diffs
-

  src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 


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


Testing
---

`make check` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.

[Jeff Coffler]

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

(Updated Feb. 12, 2018, 11:35 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Fixed Windows-specific issues (absolute path, subprocess invocation)
to allow Hadoop Fetcher Plugin to run properly.


Diffs
-

  src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 


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


Testing (updated)
---

Full test pass on both Linux and Windows.

Additionally, on Windows platform:

```
[==] Running 3 tests from 1 test case.
[--] Global test environment set-up.
[--] 3 tests from HadoopFetcherPluginTest
[ RUN  ] HadoopFetcherPluginTest.FetchExistingFile

C:\Users\jeffcof\AppData\Local\Temp\UrtxvI>if "version" == "version" (exit 0 )
[   OK ] HadoopFetcherPluginTest.FetchExistingFile (235 ms)
[ RUN  ] HadoopFetcherPluginTest.FetchNonExistingFile

C:\Users\jeffcof\AppData\Local\Temp\YJVcTA>if "version" == "version" (exit 0 )
[   OK ] HadoopFetcherPluginTest.FetchNonExistingFile (217 ms)
[ RUN  ] HadoopFetcherPluginTest.InvokeFetchByName

C:\Users\jeffcof\AppData\Local\Temp\wsQjH1>if "version" == "version" (exit 0 )
[   OK ] HadoopFetcherPluginTest.InvokeFetchByName (217 ms)
[--] 3 tests from HadoopFetcherPluginTest (674 ms total)

[--] Global test environment tear-down
[==] 3 tests from 1 test case ran. (12790 ms total)
[  PASSED  ] 3 tests.
```


Thanks,

Jeff Coffler

Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.

[Jeff Coffler]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

On Windows, used .bat (cmd.exe) script to emulate hadoop
(rather than a shell script).

Tested all `HadoopFetcherPlugin.*` tests, all succeed.


Diffs
-

  src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 


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


Testing
---


Thanks,

Jeff Coffler

Review Request 65619: Windows: Modified build system to build hadoop plugins.

[Jeff Coffler]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Modified cmake build system to build hadoop plugins.


Diffs
-

  src/CMakeLists.txt 21fb47e29dd0b19681690b8de5261c68b574a7c8 


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


Testing
---


Thanks,

Jeff Coffler

Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.

[Jeff Coffler]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Enabled both `curl` and `hadoop` fetcher plugins on Windows platform.


Diffs
-

  src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 
  src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 


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


Testing
---


Thanks,

Jeff Coffler

Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.

[Jeff Coffler]

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

Review request for mesos, Andrew Schwartzmeyer and John Kordich.


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


Repository: mesos


Description
---

Fixed Windows-specific issues (absolute path, subprocess invocation)
to allow Hadoop Fetcher Plugin to run properly.


Diffs
-

  src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 


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


Testing
---


Thanks,

Jeff Coffler

Re: Review Request 65550: Made default executor not shutdown if unsubscribed during task launch.

[Gaston Kleiman]

> On Feb. 12, 2018, 12:13 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp
> > Line 349 (original), 349 (patched)
> > 
> >
> > Not yours, but I think we can kill this. I think this was added pre 
> > multiple task groups support. We should be able to look into the 
> > `containers` map AFAICT.

Killed in https://reviews.apache.org/r/65616/


- Gaston


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


On Feb. 12, 2018, 3:13 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65550/
> ---
> 
> (Updated Feb. 12, 2018, 3:13 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would unnecessarily shutdown if, while launching a
> task group, it gets unsubscribed after having successfully launched the
> task group's containers.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65550/diff/3/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65552: Added a regression test for MESOS-8468.

[Gaston Kleiman]

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

(Updated Feb. 12, 2018, 3:24 p.m.)


Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod Kone.


Changes
---

Swapped tasks in task groups in order to prevent a potential race.


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


Repository: mesos


Description
---

Added a regression test for MESOS-8468.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp cc97e0d1fea7f4d0bc544d850593d8d91921b552 


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

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


Testing
---

`GLOG_v=1 sudo bin/mesos-tests.sh --gtest_filter='*ROOT_LaunchGroupFailure*' 
--verbose --gtest_repeat=650 --gtest_break_on_failure` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65552: Added a regression test for MESOS-8468.

[Gaston Kleiman]

> On Feb. 12, 2018, 12:35 p.m., Joseph Wu wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 3450-3461 (patched)
> > 
> >
> > Is it possible for the following race to occur?
> > 
> > * Executor launches task group 1 (expected to fail/kill)
> > * Executor performs the launch/kill.
> > * Executor commits suicide because it is no longer running any tasks.
> > * The agent sends the second task group to the now-dead executor.

Yeah, that sounds possible, I changed the test so that it now does the 
following:

1. Executor launches `taskGroup1` with a task that sleeps for a very long time 
and isn't expected to stop until killed.
2. Executor launches `taskGroup2` with a sleep task and one that should fail to 
launch.
3. Executor should kill the sleep task in `taskGroup2`.
4. Executor report all tasks in `taskGroup2` as killed/failed.
4. Scheduler will ask to kill the sole task in `taskGroup1`.
5. Executor should kill the task in `taskGroup1` and terminate.


- Gaston


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


On Feb. 12, 2018, 3:24 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65552/
> ---
> 
> (Updated Feb. 12, 2018, 3:24 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a regression test for MESOS-8468.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> cc97e0d1fea7f4d0bc544d850593d8d91921b552 
> 
> 
> Diff: https://reviews.apache.org/r/65552/diff/3/
> 
> 
> Testing
> ---
> 
> `GLOG_v=1 sudo bin/mesos-tests.sh --gtest_filter='*ROOT_LaunchGroupFailure*' 
> --verbose --gtest_repeat=650 --gtest_break_on_failure` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Feb. 12, 2018, 11:16 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

Rebased + feedback.


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


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace. The
command health check was trivially fixed by replacing the hardcoded
`sh -c`.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


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

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


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65550: Made default executor not shutdown if unsubscribed during task launch.

[Gaston Kleiman]

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

(Updated Feb. 12, 2018, 3:13 p.m.)


Review request for mesos, Anand Mazumdar, Qian Zhang, and Vinod Kone.


Changes
---

Added logging statements.


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


Repository: mesos


Description
---

The default executor would unnecessarily shutdown if, while launching a
task group, it gets unsubscribed after having successfully launched the
task group's containers.


Diffs (updated)
-

  src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 


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

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


Testing
---

`make check` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65548: Added `Event::Update` and `v1::scheduler::TaskStatus` ostream operators.

[Gaston Kleiman]

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

(Updated Feb. 12, 2018, 3:11 p.m.)


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


Changes
---

Added more fields.


Repository: mesos


Description
---

This operators make gtest print a human-readable representation of the
protos on test failures.


Diffs (updated)
-

  include/mesos/v1/mesos.hpp d4c354ab596a6ea361f2fe45afa46089f8c1a543 
  include/mesos/v1/scheduler/scheduler.hpp 
2fdd8f265d8dd5e3a334055ab8777a175688f620 
  src/v1/mesos.cpp 8abeae06e0fa87b50933df1c222ad4724a0b0116 


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

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


Testing
---

Example test failure without the patch:

```
Unexpected mock function call - returning directly.
Function call: update(0x7ffedfa94eb0, @0x7f8638002c30 32-byte object <50-92 
EA-29 87-7F 00-00 00-00 00-00 00-00 00-00 01-00 00-00 00-00 00-00 60-30 00-38 
86-7F 00-00>)
Google Mock tried the following 8 expectations, but none matched:

../../src/tests/default_executor_tests.cpp:3315: tried expectation #0: 
EXPECT_CALL(*scheduler, update(_, AllOf( 
TaskStatusUpdateTaskIdEq(sleepTaskInfo1), 
TaskStatusUpdateStateEq(v1::TASK_ERROR...
[...]
) and (task status update state eq TASK_ERROR)
   Actual: 32-byte object <50-92 EA-29 87-7F 00-00 00-00 00-00 00-00 
00-00 01-00 00-00 00-00 00-00 60-30 00-38 86-7F 00-00>
 Expected: to be called once
   Actual: never called - unsatisfied and active
```

After applying the patch:

```
Unexpected mock function call - returning directly.
Function call: update(0x7ffc6b036a20, @0x7f4af4000bb0 TASK_STARTING (Status 
UUID: f379eb50-1163-442a-8e30-a0c2f5247575) for task 'sleepTask1')
Google Mock tried the following 8 expectations, but none matched:

../../src/tests/default_executor_tests.cpp:3315: tried expectation #0: 
EXPECT_CALL(*scheduler, update(_, AllOf( 
TaskStatusUpdateTaskIdEq(sleepTaskInfo1), 
TaskStatusUpdateStateEq(v1::TASK_ERROR...
[...]
) and (task status update state eq TASK_ERROR)
   Actual: TASK_STARTING (Status UUID: 
f379eb50-1163-442a-8e30-a0c2f5247575) for task 'sleepTask1'
```


Thanks,

Gaston Kleiman

Re: Review Request 65162: Removed some redundant `get` calls.

[Benjamin Bannier]

> On Feb. 12, 2018, 8:05 p.m., Andrew Schwartzmeyer wrote:
> > I thought I'd finished reviewing because I reached the bottom of the page, 
> > but it's split across 5 pages.
> > 
> > However, this was done automatically, and the first page of changes all 
> > appear sane. Moreover, the chain passed the review bots, so I think I'm a 
> > ship-it.
> 
> Andrew Schwartzmeyer wrote:
> @bbannier, should we run the chain through Mesosphere's internal CI (the 
> one that also compiles with Clang) to make sure it's totally good?

That's a good idea, I'll update the RRs with results.


- Benjamin


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


On Feb. 12, 2018, 4:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65162/
> ---
> 
> (Updated Feb. 12, 2018, 4:55 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls.
> 
> 
> Diffs
> -
> 
>   src/appc/spec.cpp fc5e24ee5a459522a7a2dc52b4f168c4386b0642 
>   src/authentication/cram_md5/auxprop.cpp 
> 8a20d5bd2d943faa10d2920f76756fb9527075d8 
>   src/cli/execute.cpp 729896a4021a3be6dfd41a5f6bc78872170fe68a 
>   src/common/command_utils.cpp c50be7608df3a7e0a6a9d0bb46525a3cbd025bee 
>   src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 
>   src/common/parse.hpp 212d6406a16f5ffd0494197dc44271629dc5ba3b 
>   src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f 
>   src/common/recordio.hpp c58bb14866a3ec3841b09d2e96bb16af20a2e6fd 
>   src/credentials/credentials.hpp c790793c7ea5ed384bdb397bfc1592b8fd1ff327 
>   src/docker/docker.cpp 3df370e988fce12d323ff6b441da15dab27bdd28 
>   src/docker/spec.cpp 538cf1883d0dbf953ab5017d9b7d54a68ee72c73 
>   src/executor/executor.cpp 945936883afba0b52c95242cd410c335424b5768 
>   src/files/files.cpp 324b4bcf0ef53931fa7ef9b7a891b160564705ff 
>   src/hook/manager.cpp c659de5418881a77d89e6c579152614c2b47592b 
>   src/internal/evolve.cpp 7758c9b93ad41b45b941c0de8c2b1008fbc4e50d 
>   src/launcher/executor.cpp 164ecc7ba51f75db6ec581aa4b135526c304dd00 
>   src/launcher/fetcher.cpp 7fc69fe22b362bf0d13d629829a2c6a9de4f1579 
>   src/linux/cgroups.cpp ed22c456a3d12b690efccca96c619267157ee6d7 
>   src/linux/fs.cpp 94e78df0336f63c58eb3d4da0d7fe45dfcb493c1 
>   src/linux/ns.cpp fc0e7e4543903788451c44858dd2705372946ae1 
>   src/local/local.cpp ba1bcf21df70bba520f3f9b671bb57d498335c44 
>   src/log/catchup.cpp 79cc18c4311d14c82801d856681171c5f1ce95c8 
>   src/log/consensus.cpp f766015fac44cce173ec7ef3f5dd3f0ac9e03ef5 
>   src/log/replica.cpp c86a609f7fc7fcfd42761180c464c383961a986e 
>   src/log/tool/benchmark.cpp b939e26aceafc47015f3f0a93c5ce6a1f8aca605 
>   src/log/tool/initialize.cpp c0f85439a87f939bad076733fbc9e12b60f5b8f9 
>   src/log/tool/read.cpp 50fb0209a67251eb6ae320f3cd6dcd7ba11bdd99 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/contender/contender.cpp ea7453d1d4753a77c6161ec0b812bb0bf38483dd 
>   src/master/contender/zookeeper.cpp 64fbb3adbb0812f430fe152550a8a8707c0fed8f 
>   src/master/detector/detector.cpp 9d2e8c4d3fb342fe4ada01628b33b7c470ba63b6 
>   src/master/detector/zookeeper.cpp 1cab567615d756ebd1a40549718c47b642cf0eb8 
>   src/master/http.cpp 46f2872a17215464de2e4990e9cfcb3bff46812a 
>   src/master/main.cpp 0040d65ec3a77c6d69ccc3a9065c6f1996c2a1f9 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94 
>   src/master/quota_handler.cpp 37d43e1d9473b750ffd76701472e7448aa61c8c3 
>   src/master/registrar.cpp aabce22e4b06826e4452c61c27ebdd702b1e47bc 
>   src/master/validation.cpp 42f767e4269abe3f160fc7d97eb4522d1bee8e95 
>   src/master/weights_handler.cpp 59104a85d2b74cf42461b02d85101c7c751f706c 
>   src/resource_provider/http_connection.hpp 
> add5acc47aaf79b50ef9a7bd1e17ff0db5b95439 
>   src/resource_provider/registrar.cpp 
> 3e6b64bae1c56d3ca27edd0debac428b47f17d04 
>   src/sched/sched.cpp 613a33b7d3150ad851af674e989a84b3899f2d69 
>   src/scheduler/scheduler.cpp 07d2b37e0874edd27365719705272568d386323b 
>   src/slave/container_loggers/lib_logrotate.cpp 
> bc13e6a52460d4607873c4b0022165038051bc51 
>   src/slave/containerizer/containerizer.cpp 
> fa1d24a56380dd34ed15d3d12e610dfcfe4c56d6 
>   src/slave/containerizer/docker.cpp 7585178f34bdb782f0108d3ab94c23edfacc5564 
>   src/slave/containerizer/fetcher.cpp 
> 8b26e882eced9478ffdc6b2dcbc0a5796c5c3ce2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> cd1e9ae8b8925cb97396dde22b6a3eb63ab65536 
>   src/slave/containerizer/mes

Re: Review Request 65556: Made the default executor treat agent disconnections more gracefully.

[Gaston Kleiman]

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

(Updated Feb. 12, 2018, 3:08 p.m.)


Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod Kone.


Changes
---

Fixed typo.


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


Repository: mesos


Description
---

This patch makes the default executor not shutdown if there it there are
active child containers, and it fails to connect or is not subscribed to
the agent when starting to launch a task group.


Diffs (updated)
-

  src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 


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

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


Testing
---

`sudo make check` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65393: Fixed docker command health check to use the right docker socket.

[Akash Gupta]

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

(Updated Feb. 12, 2018, 10:50 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and Gaston 
Kleiman.


Changes
---

rebased.


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


Repository: mesos


Description
---

The original command health check was calling `docker exec` instead
of `docker -H  exec`, so it was ignoring the socket
value passed to the docker executor.


Diffs (updated)
-

  src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


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

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


Testing
---

ran mesos-tests --docker flag.


Thanks,

Akash Gupta

Re: Review Request 65394: Added separate structs for health check runtime and check types.

[Akash Gupta]

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

(Updated Feb. 12, 2018, 10:50 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and Gaston 
Kleiman.


Changes
---

rebased


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


Repository: mesos


Description
---

This patch is in preparation of another patch that will refactor the
health check code using these types.


Diffs (updated)
-

  src/checks/checks_runtime.hpp PRE-CREATION 
  src/checks/checks_types.hpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Akash Gupta

Re: Review Request 52064: Support for multiple versions of docs.

[Benjamin Mahler]

> On Feb. 9, 2018, 6:02 p.m., Benjamin Bannier wrote:
> > I am wondering whether it wouldn't be simpler to have the site setup just 
> > generate output for the currently checked-out version and dump that into 
> > some version-specific output folder. We could then have some CI setup 
> > execute this for the different tags or branches we care about. This 
> > wouldn't only simplify the site setup, but probably also deal better with 
> > e.g., changed requirements to the base system (e.g., needed to build 
> > binaries generating the endpoint documentation) which are currently not 
> > managed in the rake file.
> > 
> > One difficulty with that approach would be to determine under what branch 
> > we are actually working on. My guess is that we wouldn't want to update to 
> > documentation of 1.3.0 until we have tagged a 1.3.0 version, so we'd likely 
> > build documentation for the n-latest tags. The `HEAD` of `master` is more 
> > tricky as it would change more frequently and not directly have a release 
> > tag as parent (these live on release branches) making it harder to work 
> > with say `git-describe`. Maybe we could parse this from source, e.g., 
> > `MESOS_VERSION` in `include/mesos/version.hpp`.
> > 
> > The other difficulty might be that we might move a lot of site-generation 
> > setup out of the repo into e.g., Jenkins config. There are probably ways to 
> > work around that, but I haven't thought that through.
> 
> Tim Anderegg wrote:
> I'm not familiar with how Jenkins is configured and used here, but your 
> approach could be simpler depending on how complex the CI build setup is.  
> This patch would remain mostly the same, except lines 68-89 of the Rakefile 
> could be removed and the git dependency would no longer be necessary.  The 
> version could then be fed in by Jenkins, which would probably be 
> easier/simpler than trying to programmatically determine the version.  Right 
> now "master" just translates to "latest" in the patch as-is.
> 
> I saw putting everything in the Rakefile as the most straightforward 
> approach, since it allows the correct versions to generate docs for to be 
> determined programmatically, avoiding the difficulty you mentioned above. 
> However, it does require generating all documentation for all versions on 
> each build, and significant changes to the build process or documentation 
> process down the road might make it difficult to maintain a 
> backwards-compatible approach (e.g. I currently disable doc generation for 
> the oldest dozen tags or so which don't have the same documentation setup, or 
> any documentation at all).

I tend to prefer the approach Benjamin's suggested above since the 
documentation generation will be versioned alongside the documentation, so that 
we don't have to deal with backwards compatibility of documentation generation 
on HEAD.


- Benjamin


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


On Feb. 9, 2018, 1:46 a.m., Tim Anderegg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52064/
> ---
> 
> (Updated Feb. 9, 2018, 1:46 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Vinod Kone.
> 
> 
> Bugs: MESOS-3011
> https://issues.apache.org/jira/browse/MESOS-3011
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support for multiple versions of docs.
> 
> 
> Diffs
> -
> 
>   site/Gemfile 877fe914a9787c140848fdf9958571fec5fa58ff 
>   site/Gemfile.lock 909f3f3badeaa47c80929e243ce36307766edee4 
>   site/Rakefile 31ef6ffe225ce7ddc573054058af1070b9e96b09 
>   site/config.rb 04bc7aa1e0ac61ce5d89fd53d32f265532996913 
>   site/data/releases.yml e3edc308a5429585b3fc3f05564d695ba3217035 
>   site/source/assets/js/versions.js PRE-CREATION 
>   site/source/layouts/basic.erb 8a07488940f3793d6fdd291dbe896e098f321c96 
> 
> 
> Diff: https://reviews.apache.org/r/52064/diff/6/
> 
> 
> Testing
> ---
> 
> Testing was done manually to verify that the documentation was built for each 
> version of Mesos that is supported (some older versions do not have 
> compatible documentation).
> 
> 
> Thanks,
> 
> Tim Anderegg
> 
>

Re: Review Request 65393: Fixed docker command health check to use the right docker socket.

[Joseph Wu]

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


Ship it!




This shouldn't cause any problems on non-Windows (and specifying the socket is 
usually better than not specifying it).

- Joseph Wu


On Jan. 29, 2018, 10:20 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65393/
> ---
> 
> (Updated Jan. 29, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The original command health check was calling `docker exec` instead
> of `docker -H  exec`, so it was ignoring the socket
> value passed to the docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65393/diff/1/
> 
> 
> Testing
> ---
> 
> ran mesos-tests --docker flag.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65556: Made the default executor treat agent disconnections more gracefully.

[Joseph Wu]

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


Ship it!




LGTM.


src/launcher/default_executor.cpp
Line 377 (original), 385 (patched)


s/taskk/task/


- Joseph Wu


On Feb. 7, 2018, 1:55 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65556/
> ---
> 
> (Updated Feb. 7, 2018, 1:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor not shutdown if there it there are
> active child containers, and it fails to connect or is not subscribed to
> the agent when starting to launch a task group.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65556/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65593: Added a test to check default executor that failed to launch is removed.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65593 was successfully built and tested.

Reviews applied: `['65445', '65504', '65446', '65449', '65448', '65593']`

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

- Mesos Reviewbot Windows


On Feb. 9, 2018, 4:36 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65593/
> ---
> 
> (Updated Feb. 9, 2018, 4:36 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that agent sends ExitedExecutorMessage when the
> task group fails to launch due to unscheule GC failure. So that
> master's executor bookkeeping entry is removed.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65593/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65160: Removed some redundant `get` calls in stout.

[Benjamin Bannier]

> On Feb. 12, 2018, 8:12 p.m., Andrew Schwartzmeyer wrote:
> > I noticed that this didn't apply the automated reviewer to any Windows-only 
> > files. On the one hand, I have clean-up patches in progress that would be 
> > no fun to rebase afterward, on the other hand, why were those files 
> > ignored? Was it on purpose, or could I apply the same tools on Windows to 
> > automatically clean up that portion of the code base too?

These changes where generated automatically by a linter running on the AST seen 
by the compiler (https://github.com/mesos/clang-tools-extra/pull/13). This does 
require a compiler setup visiting all files with e.g., proper compiler 
switches. I was able to run this for some configurations on Linux and macos, 
but did not attempt this for a Windows configuration.


- Benjamin


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


On Feb. 12, 2018, 3:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65160/
> ---
> 
> (Updated Feb. 12, 2018, 3:07 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/flags.hpp 
> 4de093d675b094a438e38ecbc867837f7cbc5f7c 
>   3rdparty/stout/include/stout/ip.hpp 
> b4abcb963d3ac9ef4a3bda935225d5d2e52f5fd0 
>   3rdparty/stout/include/stout/json.hpp 
> 7484f4aab0025a1b6e5d51484f92b0aef96fe433 
>   3rdparty/stout/include/stout/os.hpp 
> c366ad03bbf9a850551fd824680a4afffc77782a 
>   3rdparty/stout/include/stout/os/linux.hpp 
> 50352c947d63e9babff948d90d1b7b9303d03d77 
>   3rdparty/stout/include/stout/os/osx.hpp 
> 504af3e650ab680d65107448b302ca7cf99e9ad6 
>   3rdparty/stout/include/stout/os/posix/bootid.hpp 
> 7ea5e4631b14c48455af7c9feec98bcb4ae79d75 
>   3rdparty/stout/include/stout/os/posix/fork.hpp 
> a29967dcb32c112793adb60693cfef808a52b63d 
>   3rdparty/stout/include/stout/os/posix/killtree.hpp 
> 9f18a48b94ddcbc4b3881ff4ab94c777bb10028d 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 175de46a10fb8ab2a3c14d3b70ce73de042e6387 
>   3rdparty/stout/include/stout/os/posix/which.hpp 
> 96586ddebdcdbc8a84fc5317828a474796d582ed 
>   3rdparty/stout/include/stout/os/pstree.hpp 
> fe382a19f510d10479a2f4bc8c7d0daff8ef7b36 
>   3rdparty/stout/include/stout/posix/os.hpp 
> dfee0faa084932bf60c93c7aae63665225cd4ab1 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 7f8ed34b7ecd10bb0d69e17c74a0272f0bdbbafc 
>   3rdparty/stout/include/stout/result.hpp 
> 1bf353b76b064d5901624097cfa18aa9aab05909 
>   3rdparty/stout/include/stout/try.hpp 
> 90f8aed56ce5d77e70af9e516faad152083d1488 
>   3rdparty/stout/tests/ip_tests.cpp a38e93c630bc4cbd337d8712a0edadc607382f22 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> 2204d104a5739086a8fdf6fe21032d0d12c980ad 
>   3rdparty/stout/tests/os/process_tests.cpp 
> cbae66cdb96ee5f20fc33e4f8ca2bbeeec227b00 
>   3rdparty/stout/tests/os/systems_tests.cpp 
> 44a922108b6f45ff3bd7ec08a86da6e50c43e469 
>   3rdparty/stout/tests/os_tests.cpp 11f17206688e09e86706d2d57d029f1c29d35e0d 
>   3rdparty/stout/tests/proc_tests.cpp 
> b0e3720ae199507405b66e3e9286619f84667a83 
>   3rdparty/stout/tests/some_tests.cpp 
> 685873505b68099039387e63c6faf49f3e4db4e6 
> 
> 
> Diff: https://reviews.apache.org/r/65160/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> These changes were made automatically with 
> https://github.com/mesos/clang-tools-extra/pull/13 with only a few minor 
> additions in surrounding code.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65497 was successfully built and tested.

Reviews applied: `['65613', '65497']`

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

- Mesos Reviewbot Windows


On Feb. 12, 2018, 7:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 7:48 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65551: Stopped shutting down the whole default executor on task launch failure.

[Vinod Kone]

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




src/launcher/default_executor.cpp
Line 786 (original), 788 (patched)


If a launch for nested container fails, don't we get `NotFound`?


- Vinod Kone


On Feb. 10, 2018, 6:35 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65551/
> ---
> 
> (Updated Feb. 10, 2018, 6:35 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would be completely shutdown on a
> `LAUNCH_NESTED_CONTAINER` failure.
> 
> This patch makes it kill the affected task group instead of shutting
> down and killing all task groups.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65551/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> Regression test on https://reviews.apache.org/r/65552/
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65552: Added a regression test for MESOS-8468.

[Joseph Wu]

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




src/tests/default_executor_tests.cpp
Lines 3276 (patched)


s/shoud/should/



src/tests/default_executor_tests.cpp
Lines 3450-3461 (patched)


Is it possible for the following race to occur?

* Executor launches task group 1 (expected to fail/kill)
* Executor performs the launch/kill.
* Executor commits suicide because it is no longer running any tasks.
* The agent sends the second task group to the now-dead executor.


- Joseph Wu


On Feb. 7, 2018, 12:05 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65552/
> ---
> 
> (Updated Feb. 7, 2018, 12:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a regression test for MESOS-8468.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> cc97e0d1fea7f4d0bc544d850593d8d91921b552 
> 
> 
> Diff: https://reviews.apache.org/r/65552/diff/2/
> 
> 
> Testing
> ---
> 
> `GLOG_v=1 sudo bin/mesos-tests.sh --gtest_filter='*ROOT_LaunchGroupFailure*' 
> --verbose --gtest_repeat=650 --gtest_break_on_failure` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65449: Fixed an bug where executor info lingers on master if failed to launch.

[Meng Zhu]

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

(Updated Feb. 12, 2018, 12:17 p.m.)


Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.


Changes
---

Patch updated. Thanks for the comment!


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


Repository: mesos


Description
---

Master relies on `ExitedExecutorMessage` from the agent to recycle
executor entry. However, this message won't be sent if the executor
never actually launched (due to transient error), leaving executor
info on the master lingering and resource claimed.
See MESOS-1720.

This patch fixes this issue by sending the `ExitedExecutorMessage`
from the agent if the executor is never launched.


Diffs (updated)
-

  src/slave/slave.hpp 30151c4886e12e9183a971b86b854e28a8ca1b39 
  src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 
  src/tests/mock_slave.hpp 942ead57fc67bdd2a268c67575952349838dc280 
  src/tests/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 
  src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 


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

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


Testing
---

make check
Dedicated test in #65448


Thanks,

Meng Zhu

Re: Review Request 65551: Stopped shutting down the whole default executor on task launch failure.

[Vinod Kone]

> On Feb. 12, 2018, 7:57 p.m., Joseph Wu wrote:
> > src/launcher/default_executor.cpp
> > Line 1479 (original), 1492 (patched)
> > 
> >
> > This booleans seems like a remnant of the time when the default 
> > executor would only launch a single task group.  The value is initialized 
> > to false, and is flipped to true upon launching the first TaskGroup.
> > 
> > It is `CHECK`'d in `__launchGroup(...)` (seems redundant, as that is a 
> > continuation of `launchGroup` where `launched = true;` is set); in `wait()` 
> > (similarly redundant, but has more entrypoints); and in `shutdown()`.
> > 
> > You can consider removing the bool in a separate patch.

I made the same comment in the previous review :)


- Vinod


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


On Feb. 10, 2018, 6:35 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65551/
> ---
> 
> (Updated Feb. 10, 2018, 6:35 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would be completely shutdown on a
> `LAUNCH_NESTED_CONTAINER` failure.
> 
> This patch makes it kill the affected task group instead of shutting
> down and killing all task groups.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65551/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> Regression test on https://reviews.apache.org/r/65552/
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65550: Made default executor not shutdown if unsubscribed during task launch.

[Vinod Kone]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp
Line 349 (original), 349 (patched)


Not yours, but I think we can kill this. I think this was added pre 
multiple task groups support. We should be able to look into the `containers` 
map AFAICT.



src/launcher/default_executor.cpp
Line 647 (original), 642 (patched)


Can you log that we are skipping wait in an `else` statement.



src/launcher/default_executor.cpp
Line 651 (original), 647 (patched)


Can you log here because this gets called from couple different sites?


- Vinod Kone


On Feb. 10, 2018, 6:35 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65550/
> ---
> 
> (Updated Feb. 10, 2018, 6:35 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would unnecessarily shutdown if, while launching a
> task group, it gets unsubscribed after having successfully launched the
> task group's containers.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65550/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65162: Removed some redundant `get` calls.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65160, 65161, 65162]

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

- Mesos Reviewbot


On Feb. 12, 2018, 3:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65162/
> ---
> 
> (Updated Feb. 12, 2018, 3:55 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls.
> 
> 
> Diffs
> -
> 
>   src/appc/spec.cpp fc5e24ee5a459522a7a2dc52b4f168c4386b0642 
>   src/authentication/cram_md5/auxprop.cpp 
> 8a20d5bd2d943faa10d2920f76756fb9527075d8 
>   src/cli/execute.cpp 729896a4021a3be6dfd41a5f6bc78872170fe68a 
>   src/common/command_utils.cpp c50be7608df3a7e0a6a9d0bb46525a3cbd025bee 
>   src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 
>   src/common/parse.hpp 212d6406a16f5ffd0494197dc44271629dc5ba3b 
>   src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f 
>   src/common/recordio.hpp c58bb14866a3ec3841b09d2e96bb16af20a2e6fd 
>   src/credentials/credentials.hpp c790793c7ea5ed384bdb397bfc1592b8fd1ff327 
>   src/docker/docker.cpp 3df370e988fce12d323ff6b441da15dab27bdd28 
>   src/docker/spec.cpp 538cf1883d0dbf953ab5017d9b7d54a68ee72c73 
>   src/executor/executor.cpp 945936883afba0b52c95242cd410c335424b5768 
>   src/files/files.cpp 324b4bcf0ef53931fa7ef9b7a891b160564705ff 
>   src/hook/manager.cpp c659de5418881a77d89e6c579152614c2b47592b 
>   src/internal/evolve.cpp 7758c9b93ad41b45b941c0de8c2b1008fbc4e50d 
>   src/launcher/executor.cpp 164ecc7ba51f75db6ec581aa4b135526c304dd00 
>   src/launcher/fetcher.cpp 7fc69fe22b362bf0d13d629829a2c6a9de4f1579 
>   src/linux/cgroups.cpp ed22c456a3d12b690efccca96c619267157ee6d7 
>   src/linux/fs.cpp 94e78df0336f63c58eb3d4da0d7fe45dfcb493c1 
>   src/linux/ns.cpp fc0e7e4543903788451c44858dd2705372946ae1 
>   src/local/local.cpp ba1bcf21df70bba520f3f9b671bb57d498335c44 
>   src/log/catchup.cpp 79cc18c4311d14c82801d856681171c5f1ce95c8 
>   src/log/consensus.cpp f766015fac44cce173ec7ef3f5dd3f0ac9e03ef5 
>   src/log/replica.cpp c86a609f7fc7fcfd42761180c464c383961a986e 
>   src/log/tool/benchmark.cpp b939e26aceafc47015f3f0a93c5ce6a1f8aca605 
>   src/log/tool/initialize.cpp c0f85439a87f939bad076733fbc9e12b60f5b8f9 
>   src/log/tool/read.cpp 50fb0209a67251eb6ae320f3cd6dcd7ba11bdd99 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/contender/contender.cpp ea7453d1d4753a77c6161ec0b812bb0bf38483dd 
>   src/master/contender/zookeeper.cpp 64fbb3adbb0812f430fe152550a8a8707c0fed8f 
>   src/master/detector/detector.cpp 9d2e8c4d3fb342fe4ada01628b33b7c470ba63b6 
>   src/master/detector/zookeeper.cpp 1cab567615d756ebd1a40549718c47b642cf0eb8 
>   src/master/http.cpp 46f2872a17215464de2e4990e9cfcb3bff46812a 
>   src/master/main.cpp 0040d65ec3a77c6d69ccc3a9065c6f1996c2a1f9 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94 
>   src/master/quota_handler.cpp 37d43e1d9473b750ffd76701472e7448aa61c8c3 
>   src/master/registrar.cpp aabce22e4b06826e4452c61c27ebdd702b1e47bc 
>   src/master/validation.cpp 42f767e4269abe3f160fc7d97eb4522d1bee8e95 
>   src/master/weights_handler.cpp 59104a85d2b74cf42461b02d85101c7c751f706c 
>   src/resource_provider/http_connection.hpp 
> add5acc47aaf79b50ef9a7bd1e17ff0db5b95439 
>   src/resource_provider/registrar.cpp 
> 3e6b64bae1c56d3ca27edd0debac428b47f17d04 
>   src/sched/sched.cpp 613a33b7d3150ad851af674e989a84b3899f2d69 
>   src/scheduler/scheduler.cpp 07d2b37e0874edd27365719705272568d386323b 
>   src/slave/container_loggers/lib_logrotate.cpp 
> bc13e6a52460d4607873c4b0022165038051bc51 
>   src/slave/containerizer/containerizer.cpp 
> fa1d24a56380dd34ed15d3d12e610dfcfe4c56d6 
>   src/slave/containerizer/docker.cpp 7585178f34bdb782f0108d3ab94c23edfacc5564 
>   src/slave/containerizer/fetcher.cpp 
> 8b26e882eced9478ffdc6b2dcbc0a5796c5c3ce2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> cd1e9ae8b8925cb97396dde22b6a3eb63ab65536 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> ba7740403cdb9c3a35ee366d09503807a5cea27a 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> fc763bd7834567882146ad25e0266b1183154dc3 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> ffa0d3b0e6f3d8a1f55c88d61f20cff426119e20 
>   src/slave/con

Re: Review Request 64211: Added options to build the Python CLI and run unit tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64211 was successfully built and tested.

Reviews applied: `['65584', '65585', '64211']`

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

- Mesos Reviewbot Windows


On Feb. 11, 2018, 3:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64211/
> ---
> 
> (Updated Feb. 11, 2018, 3:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An update of the discarded review /r/52543.
> 
> Works with Autotools and CMake.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 50cddf9476c8c5196c4824a7b060c2680a96b277 
>   cmake/MesosConfigure.cmake 0954a9cd31fa290ff9099be4b06d69d96b701f1e 
>   configure.ac 30fbadc32d1d96f719d45fa8067f975283c25507 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 21fb47e29dd0b19681690b8de5261c68b574a7c8 
>   src/Makefile.am 45f0480be0ccb9d9adf90a8a7468eb2dadc84151 
>   src/python/cli_new/CMakeLists.txt PRE-CREATION 
>   src/python/cli_new/tests/CMakeLists.txt PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64211/diff/6/
> 
> 
> Testing
> ---
> 
> Testing done on Fedora 25.
> 
> For Autotools:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ ../configure --enable-new-cli --disable-java
> $ make check
> ```
> 
> For CMake:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ cmake .. -DENABLE_NEW_CLI=1
> $ cmake --build . -- -j16
> ```
> 
> Checked that the the CLI tests were run, that the content of the directory 
> `build/src/cli` was as expected, and that `build/src/mesos` was correctly 
> running.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Meng Zhu]

> On Feb. 7, 2018, 6:19 p.m., Benjamin Mahler wrote:
> > src/tests/mock_slave.cpp
> > Lines 107-108 (original), 107-109 (patched)
> > 
> >
> > What's going on here?
> 
> Meng Zhu wrote:
> For the agent failover test, I need the failed agent to start with the 
> same pid. There is a parameter `id` you can pass to the Slave constructor 
> above. But it turns out the original code ignore that argument and just call 
> `process::ID::generate("slave")` every time, changed it to take the argument.
> 
> Benjamin Mahler wrote:
> I see, so this was a bug? Can you fix this in a separate patch?
> 
> I also didn't follow why you needed to add the `ProcessBase` constructor 
> call, since that's done within the slave constructor already? What does it 
> mean for it to happen twice?

OK, separate patch for `id` initialization in #65613.


> On Feb. 7, 2018, 6:19 p.m., Benjamin Mahler wrote:
> > src/tests/slave_tests.cpp
> > Lines 4926-4927 (patched)
> > 
> >
> > even after the exeutor has been registered..? I think you want to 
> > remove that?
> 
> Meng Zhu wrote:
> Why? The agent failover happens after the executor has been registered. 
> Then the executor re-registers and got killed.
> 
> Benjamin Mahler wrote:
> Oh I read it as the executor being registered after failover rather than 
> before, how about:
> 
> ```
> // This test verifies that if an agent fails over after registering
> // a v1 executor but before delivering its initial task groups, the
> // executor will be shut down since all of its initial task groups
> // were dropped. See MESOS-8411.
> ```
> 
> This also avoids the hanging sentence at the end that mentions task group 
> by stating it within the description.

Sounds good. Thank you!


> On Feb. 7, 2018, 6:19 p.m., Benjamin Mahler wrote:
> > src/tests/slave_tests.cpp
> > Lines 4945 (patched)
> > 
> >
> > "NotSlave"?
> > 
> > We should probably have found a way to use the logic from 
> > Master::newSlaveId here.
> 
> Meng Zhu wrote:
> Changed it to "slave". That we will need to change cluster::master, I do 
> not think it is important here. Let's just use "slave".
> 
> Benjamin Mahler wrote:
> Oh, I mistook this for the SlaveID rather than the PID::id. I'm not sure 
> I followed your response, but re-using the master code isn't applicable here 
> anyway.
> 
> To avoid this confusion for other readers, can you do the following?
> 
> ```
> string processId = process::ID::generate("slave");
> 
> StartSlave(..., processId, ...);
> ```

Sounds good.


- Meng


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


On Feb. 12, 2018, 11:48 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 11:48 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Meng Zhu]

> On Feb. 9, 2018, 5:17 p.m., Benjamin Mahler wrote:
> > src/tests/mesos.cpp
> > Line 391 (original), 400 (patched)
> > 
> >
> > I realize this is a copy/paste, but do you know why we don't start() 
> > when mock is true?

If we want to set up expect calls for the agent start code path, we need to (1) 
create the mock slave instance; (2) setup expectations; (3) start the slave.


- Meng


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


On Feb. 12, 2018, 11:48 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 12, 2018, 11:48 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
>   src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65551: Stopped shutting down the whole default executor on task launch failure.

[Joseph Wu]

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



Mostly LGTM... some suggestions:


src/launcher/default_executor.cpp
Line 538 (original), 530 (patched)


Consider adding:
`CHECK_EQ(containerIds.size(), reponses->size());`



src/launcher/default_executor.cpp
Line 1479 (original), 1492 (patched)


This booleans seems like a remnant of the time when the default executor 
would only launch a single task group.  The value is initialized to false, and 
is flipped to true upon launching the first TaskGroup.

It is `CHECK`'d in `__launchGroup(...)` (seems redundant, as that is a 
continuation of `launchGroup` where `launched = true;` is set); in `wait()` 
(similarly redundant, but has more entrypoints); and in `shutdown()`.

You can consider removing the bool in a separate patch.


- Joseph Wu


On Feb. 9, 2018, 10:35 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65551/
> ---
> 
> (Updated Feb. 9, 2018, 10:35 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor would be completely shutdown on a
> `LAUNCH_NESTED_CONTAINER` failure.
> 
> This patch makes it kill the affected task group instead of shutting
> down and killing all task groups.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65551/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> Regression test on https://reviews.apache.org/r/65552/
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Meng Zhu]

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

(Updated Feb. 12, 2018, 11:48 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Patch updated. Thanks for the comments!


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


Repository: mesos


Description
---

This test verifies that the v1 executor is shutdown if all of its
initial tasks could not be delivered when re-subscribing with
a recovered agent. See MESOS-8411.


Diffs (updated)
-

  src/tests/mesos.hpp c5593c25dde18c005f195a1885a8586bc72c849f 
  src/tests/mesos.cpp ac789297d6aa034121b73e4efc0aafee1ee3b60f 
  src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 65593: Added a test to ensure master removes fail-launched deafult executor.

[Vinod Kone]

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


Fix it, then Ship it!




Description: s/fail-launched deafult/default executor that failed to launch/


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


s/unscheule/unschedule/
s/.So that/and that/


- Vinod Kone


On Feb. 10, 2018, 12:36 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65593/
> ---
> 
> (Updated Feb. 10, 2018, 12:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that agent sends ExitedExecutorMessage when the
> task group fails to launch due to unscheule GC failure. So that
> master's executor bookkeeping entry is removed.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 628b0d0fc862264c9553e1660c7df548df9cd4a1 
> 
> 
> Diff: https://reviews.apache.org/r/65593/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65549: Improved some default executor log messages.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 10, 2018, 6:34 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65549/
> ---
> 
> (Updated Feb. 10, 2018, 6:34 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8468
> https://issues.apache.org/jira/browse/MESOS-8468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved some default executor log messages.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
> 
> 
> Diff: https://reviews.apache.org/r/65549/diff/2/
> 
> 
> Testing
> ---
> 
> None, this patch doesn't contain functional changes.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['65594']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(2601):
 warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3091):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3426):
 warning C4996: 'strerror': This function or variable may be unsafe. Consider 
using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxp

Re: Review Request 65449: Fixed an bug where executor info lingers on master if failed to launch.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/slave.cpp
Line 1909 (original), 1916 (patched)


Can you add a comment here on why you are not sending 
`ExitedExecutorMessage`? Is it because the expectation is that agent will 
(eventually) re-register and reconcile the state in this scenario?



src/slave/slave.cpp
Lines 2678 (patched)


s/task,/task;/



src/slave/slave.cpp
Lines 2692 (patched)


s/received/it was received/



src/slave/slave.cpp
Lines 2738 (patched)


There is another step for us to trigger this right?

```
(4) Master now sends another `runTaskMessage` for the same executor id with 
`launch_executor = true`.
```



src/slave/slave.cpp
Lines 2759 (patched)


s/launch the executor/launch the executor but the master doesn't know about 
it yet because the `ExitedExecutorMessage` is still in flight./



src/slave/slave.cpp
Lines 2785 (patched)


Should we be sending an `ExitedExecutorMessage` in this case to be safe? I 
guess not strictly require since the expectation is that one is already in 
flight and if it gets dropped we will hopefully reconcile. Worth adding a 
comment though for posterity.



src/slave/slave.cpp
Lines 2789 (patched)


Ideally we should have tests for each of these scenarios here. Do we?



src/slave/slave.cpp
Lines 3402 (patched)


"Consider doing a `CHECK` here since this shouldn't be possible."


- Vinod Kone


On Feb. 5, 2018, 3 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65449/
> ---
> 
> (Updated Feb. 5, 2018, 3 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master relies on `ExitedExecutorMessage` from the agent to recycle
> executor entry. However, this message won't be sent if the executor
> never actually launched (due to transient error), leaving executor
> info on the master lingering and resource claimed.
> See MESOS-1720.
> 
> This patch fixes this issue by sending the `ExitedExecutorMessage`
> from the agent if the executor is never launched.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 30151c4886e12e9183a971b86b854e28a8ca1b39 
>   src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 
>   src/tests/mock_slave.hpp 942ead57fc67bdd2a268c67575952349838dc280 
>   src/tests/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 
>   src/tests/slave_tests.cpp b54688add9949b9c4ac2ce3a42503a411e6da09f 
> 
> 
> Diff: https://reviews.apache.org/r/65449/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated test in #65448
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 65613: Fixed a bug where MockSlave `id` is not properly initialized.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

The `MockSlave` did not initialize the id field using the input
argument. Also it is necessary to call its virtual base class
`ProcessBase` constructor explicitly to avoid argument `id` being
lost.


Diffs
-

  src/tests/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 52064: Support for multiple versions of docs.

[Tim Anderegg]

> On Feb. 9, 2018, 6:02 p.m., Benjamin Bannier wrote:
> > I am wondering whether it wouldn't be simpler to have the site setup just 
> > generate output for the currently checked-out version and dump that into 
> > some version-specific output folder. We could then have some CI setup 
> > execute this for the different tags or branches we care about. This 
> > wouldn't only simplify the site setup, but probably also deal better with 
> > e.g., changed requirements to the base system (e.g., needed to build 
> > binaries generating the endpoint documentation) which are currently not 
> > managed in the rake file.
> > 
> > One difficulty with that approach would be to determine under what branch 
> > we are actually working on. My guess is that we wouldn't want to update to 
> > documentation of 1.3.0 until we have tagged a 1.3.0 version, so we'd likely 
> > build documentation for the n-latest tags. The `HEAD` of `master` is more 
> > tricky as it would change more frequently and not directly have a release 
> > tag as parent (these live on release branches) making it harder to work 
> > with say `git-describe`. Maybe we could parse this from source, e.g., 
> > `MESOS_VERSION` in `include/mesos/version.hpp`.
> > 
> > The other difficulty might be that we might move a lot of site-generation 
> > setup out of the repo into e.g., Jenkins config. There are probably ways to 
> > work around that, but I haven't thought that through.

I'm not familiar with how Jenkins is configured and used here, but your 
approach could be simpler depending on how complex the CI build setup is.  This 
patch would remain mostly the same, except lines 68-89 of the Rakefile could be 
removed and the git dependency would no longer be necessary.  The version could 
then be fed in by Jenkins, which would probably be easier/simpler than trying 
to programmatically determine the version.  Right now "master" just translates 
to "latest" in the patch as-is.

I saw putting everything in the Rakefile as the most straightforward approach, 
since it allows the correct versions to generate docs for to be determined 
programmatically, avoiding the difficulty you mentioned above. However, it does 
require generating all documentation for all versions on each build, and 
significant changes to the build process or documentation process down the road 
might make it difficult to maintain a backwards-compatible approach (e.g. I 
currently disable doc generation for the oldest dozen tags or so which don't 
have the same documentation setup, or any documentation at all).


- Tim


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


On Feb. 9, 2018, 1:46 a.m., Tim Anderegg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52064/
> ---
> 
> (Updated Feb. 9, 2018, 1:46 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Vinod Kone.
> 
> 
> Bugs: MESOS-3011
> https://issues.apache.org/jira/browse/MESOS-3011
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support for multiple versions of docs.
> 
> 
> Diffs
> -
> 
>   site/Gemfile 877fe914a9787c140848fdf9958571fec5fa58ff 
>   site/Gemfile.lock 909f3f3badeaa47c80929e243ce36307766edee4 
>   site/Rakefile 31ef6ffe225ce7ddc573054058af1070b9e96b09 
>   site/config.rb 04bc7aa1e0ac61ce5d89fd53d32f265532996913 
>   site/data/releases.yml e3edc308a5429585b3fc3f05564d695ba3217035 
>   site/source/assets/js/versions.js PRE-CREATION 
>   site/source/layouts/basic.erb 8a07488940f3793d6fdd291dbe896e098f321c96 
> 
> 
> Diff: https://reviews.apache.org/r/52064/diff/6/
> 
> 
> Testing
> ---
> 
> Testing was done manually to verify that the documentation was built for each 
> version of Mesos that is supported (some older versions do not have 
> compatible documentation).
> 
> 
> Thanks,
> 
> Tim Anderegg
> 
>

Re: Review Request 65395: Refactored health checks to cleanly separate each different check.

[Andrew Schwartzmeyer]

> On Feb. 8, 2018, 2:49 p.m., Gaston Kleiman wrote:
> > src/checks/checker_process.cpp
> > Lines 333-339 (patched)
> > 
> >
> > In my opinion the following is easier to understand:
> > 
> > ```
> > #ifdef __WINDOWS__
> > // Case E
> > return dockerHttpCheck(http, d);
> > #else
> > // Case B*
> > return httpCheck(http, runtime::Plain{d.namespaces, 
> > d.taskPid});
> > #endif // __WINDOWS__
> > ```
> > 
> > And I'd do the same elsewhere.
> > 
> > I wonder what Andy and Alex think about this.
> 
> Alexander Rukletsov wrote:
> Alex does not have a strong opinion, but tends to avoid negation if 
> possible.

Andy's opinion, in this case, is to use `#ifdef Windows / #else`, because I 
also avoid negation where possible.

However, there are places where `#ifndef Windows` is required because `#ifdef 
Linux` isn't correct. This just isn't one of them.


- Andrew


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


On Feb. 8, 2018, 9:49 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> ---
> 
> (Updated Feb. 8, 2018, 9:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored health check code to separate the logic for each check
> type and runtime (Plain/Docker/Nested). Now the matrix of different
> possiblilites is cleanly enumerated, making it easier to add
> future health checks, such as the ones for Windows.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/65395/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated Feb. 12, 2018, 7:07 p.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and no
longer assumes that profiles won't be removed by the disk profile
adaptor. It will reject volume/block creation requests that use
missing profiles, but existing volumes/blocks will continue to work.
When a volume/block with a missing profiles is destroyed, it will be
converted to an empty resource, and SLRP will update the sizes of
storage pools for currently known profiles.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
33abc0e05a804969ae14da9cb9c58698ba1aaea5 


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

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


Testing (updated)
---

sudo make check

NOTE: This patch does not change the URI disk profile daptor to allow missing 
profiles from upstream. Will have another patch to address this.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65161: Removed some redundant `get` calls in libprocess.

[Andrew Schwartzmeyer]

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


Ship it!





3rdparty/libprocess/include/process/gtest.hpp
Lines 429-430 (original), 429-430 (patched)


Ha! It's funny it caught the one in the string.


- Andrew Schwartzmeyer


On Feb. 12, 2018, 6:07 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65161/
> ---
> 
> (Updated Feb. 12, 2018, 6:07 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp 
> 231efcd989b829b5cfba8e5d69ca5cb2c4bf0428 
>   3rdparty/libprocess/include/process/gtest.hpp 
> eee726653d52af2e4a148819e420ebd22e5123a9 
>   3rdparty/libprocess/include/process/system.hpp 
> 21bd3300b104eaa56642f19c3dcb95950ab94830 
>   3rdparty/libprocess/src/http.cpp 76a65e02fd83219b198d0b8fde3f8208e1c606b5 
>   3rdparty/libprocess/src/http_proxy.cpp 
> f584238aadd86875d7c87736653f394e716397de 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> f79541853b4c826014ee969633345c3d51520ecf 
>   3rdparty/libprocess/src/openssl.cpp 
> f2fe90bfdf87dbc674b231f6392e95a95ebd9736 
>   3rdparty/libprocess/src/process.cpp 
> 42e7adf740b234ebf23d2dcb71440749c0ed87ec 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 9b1d6229c3aa7d7045d492d377223d56bca16156 
>   3rdparty/libprocess/src/subprocess_posix.hpp 
> 25eb44ab7f68907c30008ae729403a986cb1898a 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 48b5f5cf38d8fb4d77025f9723031075ca8ab677 
>   3rdparty/libprocess/src/tests/collect_tests.cpp 
> 7c2ba902d2fba340a5fae49a911a9e84223fe1ce 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> bd98154cfa2020b7423951f070fca7b0447d3cd5 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> a60f6b132aff00da42e733f5c5f387dff2428ed2 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp 
> 5a5a2e581d0917ad5aa6aa93257ec0dddc895725 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 96b1675dea7df675667c61ff2bd1d1cc8231a457 
>   3rdparty/libprocess/src/tests/reap_tests.cpp 
> f8a8593b685c5e5a9321d970a57e9ebdff127c24 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 71f2df8cd2377ae1be762ec7a0fb0d2f7b210e4b 
>   3rdparty/libprocess/src/tests/statistics_tests.cpp 
> 144b5109cfb7640b29bec8de8f5b2ad00665212f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 8211ae63e75359705af2bb9ac02a7cbdb28e3252 
>   3rdparty/libprocess/src/tests/system_tests.cpp 
> 6beb973e079c00ba3e632d6fb6c3c332483d203f 
>   3rdparty/libprocess/src/tests/timeseries_tests.cpp 
> 4177fb673bbf510fe835af396cd76dae48a9ec85 
> 
> 
> Diff: https://reviews.apache.org/r/65161/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> These changes were made automatically with 
> https://github.com/mesos/clang-tools-extra/pull/13 with only a few minor 
> additions in surrounding code.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65162: Removed some redundant `get` calls.

[Andrew Schwartzmeyer]

> On Feb. 12, 2018, 11:05 a.m., Andrew Schwartzmeyer wrote:
> > I thought I'd finished reviewing because I reached the bottom of the page, 
> > but it's split across 5 pages.
> > 
> > However, this was done automatically, and the first page of changes all 
> > appear sane. Moreover, the chain passed the review bots, so I think I'm a 
> > ship-it.

@bbannier, should we run the chain through Mesosphere's internal CI (the one 
that also compiles with Clang) to make sure it's totally good?


- Andrew


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


On Feb. 12, 2018, 7:55 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65162/
> ---
> 
> (Updated Feb. 12, 2018, 7:55 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls.
> 
> 
> Diffs
> -
> 
>   src/appc/spec.cpp fc5e24ee5a459522a7a2dc52b4f168c4386b0642 
>   src/authentication/cram_md5/auxprop.cpp 
> 8a20d5bd2d943faa10d2920f76756fb9527075d8 
>   src/cli/execute.cpp 729896a4021a3be6dfd41a5f6bc78872170fe68a 
>   src/common/command_utils.cpp c50be7608df3a7e0a6a9d0bb46525a3cbd025bee 
>   src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 
>   src/common/parse.hpp 212d6406a16f5ffd0494197dc44271629dc5ba3b 
>   src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f 
>   src/common/recordio.hpp c58bb14866a3ec3841b09d2e96bb16af20a2e6fd 
>   src/credentials/credentials.hpp c790793c7ea5ed384bdb397bfc1592b8fd1ff327 
>   src/docker/docker.cpp 3df370e988fce12d323ff6b441da15dab27bdd28 
>   src/docker/spec.cpp 538cf1883d0dbf953ab5017d9b7d54a68ee72c73 
>   src/executor/executor.cpp 945936883afba0b52c95242cd410c335424b5768 
>   src/files/files.cpp 324b4bcf0ef53931fa7ef9b7a891b160564705ff 
>   src/hook/manager.cpp c659de5418881a77d89e6c579152614c2b47592b 
>   src/internal/evolve.cpp 7758c9b93ad41b45b941c0de8c2b1008fbc4e50d 
>   src/launcher/executor.cpp 164ecc7ba51f75db6ec581aa4b135526c304dd00 
>   src/launcher/fetcher.cpp 7fc69fe22b362bf0d13d629829a2c6a9de4f1579 
>   src/linux/cgroups.cpp ed22c456a3d12b690efccca96c619267157ee6d7 
>   src/linux/fs.cpp 94e78df0336f63c58eb3d4da0d7fe45dfcb493c1 
>   src/linux/ns.cpp fc0e7e4543903788451c44858dd2705372946ae1 
>   src/local/local.cpp ba1bcf21df70bba520f3f9b671bb57d498335c44 
>   src/log/catchup.cpp 79cc18c4311d14c82801d856681171c5f1ce95c8 
>   src/log/consensus.cpp f766015fac44cce173ec7ef3f5dd3f0ac9e03ef5 
>   src/log/replica.cpp c86a609f7fc7fcfd42761180c464c383961a986e 
>   src/log/tool/benchmark.cpp b939e26aceafc47015f3f0a93c5ce6a1f8aca605 
>   src/log/tool/initialize.cpp c0f85439a87f939bad076733fbc9e12b60f5b8f9 
>   src/log/tool/read.cpp 50fb0209a67251eb6ae320f3cd6dcd7ba11bdd99 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/contender/contender.cpp ea7453d1d4753a77c6161ec0b812bb0bf38483dd 
>   src/master/contender/zookeeper.cpp 64fbb3adbb0812f430fe152550a8a8707c0fed8f 
>   src/master/detector/detector.cpp 9d2e8c4d3fb342fe4ada01628b33b7c470ba63b6 
>   src/master/detector/zookeeper.cpp 1cab567615d756ebd1a40549718c47b642cf0eb8 
>   src/master/http.cpp 46f2872a17215464de2e4990e9cfcb3bff46812a 
>   src/master/main.cpp 0040d65ec3a77c6d69ccc3a9065c6f1996c2a1f9 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94 
>   src/master/quota_handler.cpp 37d43e1d9473b750ffd76701472e7448aa61c8c3 
>   src/master/registrar.cpp aabce22e4b06826e4452c61c27ebdd702b1e47bc 
>   src/master/validation.cpp 42f767e4269abe3f160fc7d97eb4522d1bee8e95 
>   src/master/weights_handler.cpp 59104a85d2b74cf42461b02d85101c7c751f706c 
>   src/resource_provider/http_connection.hpp 
> add5acc47aaf79b50ef9a7bd1e17ff0db5b95439 
>   src/resource_provider/registrar.cpp 
> 3e6b64bae1c56d3ca27edd0debac428b47f17d04 
>   src/sched/sched.cpp 613a33b7d3150ad851af674e989a84b3899f2d69 
>   src/scheduler/scheduler.cpp 07d2b37e0874edd27365719705272568d386323b 
>   src/slave/container_loggers/lib_logrotate.cpp 
> bc13e6a52460d4607873c4b0022165038051bc51 
>   src/slave/containerizer/containerizer.cpp 
> fa1d24a56380dd34ed15d3d12e610dfcfe4c56d6 
>   src/slave/containerizer/docker.cpp 7585178f34bdb782f0108d3ab94c23edfacc5564 
>   src/slave/containerizer/fetcher.cpp 
> 8b26e882eced9478ffdc6b2dcbc0a5796c5c3ce2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> cd1e9ae8b8925cb97396dde22b6a3eb63ab65536 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> ba7740403cdb9c3a35ee366d09503807a5cea27a 
>   src/slave/containerizer/m

Re: Review Request 65160: Removed some redundant `get` calls in stout.

[Andrew Schwartzmeyer]

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


Ship it!




I noticed that this didn't apply the automated reviewer to any Windows-only 
files. On the one hand, I have clean-up patches in progress that would be no 
fun to rebase afterward, on the other hand, why were those files ignored? Was 
it on purpose, or could I apply the same tools on Windows to automatically 
clean up that portion of the code base too?

- Andrew Schwartzmeyer


On Feb. 12, 2018, 6:07 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65160/
> ---
> 
> (Updated Feb. 12, 2018, 6:07 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/flags.hpp 
> 4de093d675b094a438e38ecbc867837f7cbc5f7c 
>   3rdparty/stout/include/stout/ip.hpp 
> b4abcb963d3ac9ef4a3bda935225d5d2e52f5fd0 
>   3rdparty/stout/include/stout/json.hpp 
> 7484f4aab0025a1b6e5d51484f92b0aef96fe433 
>   3rdparty/stout/include/stout/os.hpp 
> c366ad03bbf9a850551fd824680a4afffc77782a 
>   3rdparty/stout/include/stout/os/linux.hpp 
> 50352c947d63e9babff948d90d1b7b9303d03d77 
>   3rdparty/stout/include/stout/os/osx.hpp 
> 504af3e650ab680d65107448b302ca7cf99e9ad6 
>   3rdparty/stout/include/stout/os/posix/bootid.hpp 
> 7ea5e4631b14c48455af7c9feec98bcb4ae79d75 
>   3rdparty/stout/include/stout/os/posix/fork.hpp 
> a29967dcb32c112793adb60693cfef808a52b63d 
>   3rdparty/stout/include/stout/os/posix/killtree.hpp 
> 9f18a48b94ddcbc4b3881ff4ab94c777bb10028d 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 175de46a10fb8ab2a3c14d3b70ce73de042e6387 
>   3rdparty/stout/include/stout/os/posix/which.hpp 
> 96586ddebdcdbc8a84fc5317828a474796d582ed 
>   3rdparty/stout/include/stout/os/pstree.hpp 
> fe382a19f510d10479a2f4bc8c7d0daff8ef7b36 
>   3rdparty/stout/include/stout/posix/os.hpp 
> dfee0faa084932bf60c93c7aae63665225cd4ab1 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 7f8ed34b7ecd10bb0d69e17c74a0272f0bdbbafc 
>   3rdparty/stout/include/stout/result.hpp 
> 1bf353b76b064d5901624097cfa18aa9aab05909 
>   3rdparty/stout/include/stout/try.hpp 
> 90f8aed56ce5d77e70af9e516faad152083d1488 
>   3rdparty/stout/tests/ip_tests.cpp a38e93c630bc4cbd337d8712a0edadc607382f22 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> 2204d104a5739086a8fdf6fe21032d0d12c980ad 
>   3rdparty/stout/tests/os/process_tests.cpp 
> cbae66cdb96ee5f20fc33e4f8ca2bbeeec227b00 
>   3rdparty/stout/tests/os/systems_tests.cpp 
> 44a922108b6f45ff3bd7ec08a86da6e50c43e469 
>   3rdparty/stout/tests/os_tests.cpp 11f17206688e09e86706d2d57d029f1c29d35e0d 
>   3rdparty/stout/tests/proc_tests.cpp 
> b0e3720ae199507405b66e3e9286619f84667a83 
>   3rdparty/stout/tests/some_tests.cpp 
> 685873505b68099039387e63c6faf49f3e4db4e6 
> 
> 
> Diff: https://reviews.apache.org/r/65160/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> These changes were made automatically with 
> https://github.com/mesos/clang-tools-extra/pull/13 with only a few minor 
> additions in surrounding code.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On Feb. 10, 2018, 2:22 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/uri_disk_profile.cpp
> > Line 248 (original), 242 (patched)
> > 
> >
> > This should be removed to support missing profiles from upstream.

Will address this in a follow-up patch.


- Chun-Hung


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


On Feb. 10, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated Feb. 10, 2018, 12:47 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and no
> longer assumes that profiles won't be removed by the disk profile
> adaptor. It will reject volume/block creation requests that use
> missing profiles, but existing volumes/blocks will continue to work.
> When a volume/block with a missing profiles is destroyed, it will be
> converted to an empty resource, and SLRP will update the sizes of
> storage pools for currently known profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 33abc0e05a804969ae14da9cb9c58698ba1aaea5 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 665798fdb085ea34f93bd287fe6f9ab29a265cbf 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65162: Removed some redundant `get` calls.

[Andrew Schwartzmeyer]

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


Ship it!




I thought I'd finished reviewing because I reached the bottom of the page, but 
it's split across 5 pages.

However, this was done automatically, and the first page of changes all appear 
sane. Moreover, the chain passed the review bots, so I think I'm a ship-it.


src/linux/cgroups.cpp
Lines 2410-2419 (original), 2410-2418 (patched)


It irks me that we have a C-style `(double)` cast here (and a `pid_t()` 
function-style cast above, though perhaps that one irks me less). But it's not 
something to fix in this patch.


- Andrew Schwartzmeyer


On Feb. 12, 2018, 7:55 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65162/
> ---
> 
> (Updated Feb. 12, 2018, 7:55 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some redundant `get` calls.
> 
> 
> Diffs
> -
> 
>   src/appc/spec.cpp fc5e24ee5a459522a7a2dc52b4f168c4386b0642 
>   src/authentication/cram_md5/auxprop.cpp 
> 8a20d5bd2d943faa10d2920f76756fb9527075d8 
>   src/cli/execute.cpp 729896a4021a3be6dfd41a5f6bc78872170fe68a 
>   src/common/command_utils.cpp c50be7608df3a7e0a6a9d0bb46525a3cbd025bee 
>   src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 
>   src/common/parse.hpp 212d6406a16f5ffd0494197dc44271629dc5ba3b 
>   src/common/protobuf_utils.cpp b5c2997ada8362e42150fa3cfd762120e5ea715f 
>   src/common/recordio.hpp c58bb14866a3ec3841b09d2e96bb16af20a2e6fd 
>   src/credentials/credentials.hpp c790793c7ea5ed384bdb397bfc1592b8fd1ff327 
>   src/docker/docker.cpp 3df370e988fce12d323ff6b441da15dab27bdd28 
>   src/docker/spec.cpp 538cf1883d0dbf953ab5017d9b7d54a68ee72c73 
>   src/executor/executor.cpp 945936883afba0b52c95242cd410c335424b5768 
>   src/files/files.cpp 324b4bcf0ef53931fa7ef9b7a891b160564705ff 
>   src/hook/manager.cpp c659de5418881a77d89e6c579152614c2b47592b 
>   src/internal/evolve.cpp 7758c9b93ad41b45b941c0de8c2b1008fbc4e50d 
>   src/launcher/executor.cpp 164ecc7ba51f75db6ec581aa4b135526c304dd00 
>   src/launcher/fetcher.cpp 7fc69fe22b362bf0d13d629829a2c6a9de4f1579 
>   src/linux/cgroups.cpp ed22c456a3d12b690efccca96c619267157ee6d7 
>   src/linux/fs.cpp 94e78df0336f63c58eb3d4da0d7fe45dfcb493c1 
>   src/linux/ns.cpp fc0e7e4543903788451c44858dd2705372946ae1 
>   src/local/local.cpp ba1bcf21df70bba520f3f9b671bb57d498335c44 
>   src/log/catchup.cpp 79cc18c4311d14c82801d856681171c5f1ce95c8 
>   src/log/consensus.cpp f766015fac44cce173ec7ef3f5dd3f0ac9e03ef5 
>   src/log/replica.cpp c86a609f7fc7fcfd42761180c464c383961a986e 
>   src/log/tool/benchmark.cpp b939e26aceafc47015f3f0a93c5ce6a1f8aca605 
>   src/log/tool/initialize.cpp c0f85439a87f939bad076733fbc9e12b60f5b8f9 
>   src/log/tool/read.cpp 50fb0209a67251eb6ae320f3cd6dcd7ba11bdd99 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/contender/contender.cpp ea7453d1d4753a77c6161ec0b812bb0bf38483dd 
>   src/master/contender/zookeeper.cpp 64fbb3adbb0812f430fe152550a8a8707c0fed8f 
>   src/master/detector/detector.cpp 9d2e8c4d3fb342fe4ada01628b33b7c470ba63b6 
>   src/master/detector/zookeeper.cpp 1cab567615d756ebd1a40549718c47b642cf0eb8 
>   src/master/http.cpp 46f2872a17215464de2e4990e9cfcb3bff46812a 
>   src/master/main.cpp 0040d65ec3a77c6d69ccc3a9065c6f1996c2a1f9 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94 
>   src/master/quota_handler.cpp 37d43e1d9473b750ffd76701472e7448aa61c8c3 
>   src/master/registrar.cpp aabce22e4b06826e4452c61c27ebdd702b1e47bc 
>   src/master/validation.cpp 42f767e4269abe3f160fc7d97eb4522d1bee8e95 
>   src/master/weights_handler.cpp 59104a85d2b74cf42461b02d85101c7c751f706c 
>   src/resource_provider/http_connection.hpp 
> add5acc47aaf79b50ef9a7bd1e17ff0db5b95439 
>   src/resource_provider/registrar.cpp 
> 3e6b64bae1c56d3ca27edd0debac428b47f17d04 
>   src/sched/sched.cpp 613a33b7d3150ad851af674e989a84b3899f2d69 
>   src/scheduler/scheduler.cpp 07d2b37e0874edd27365719705272568d386323b 
>   src/slave/container_loggers/lib_logrotate.cpp 
> bc13e6a52460d4607873c4b0022165038051bc51 
>   src/slave/containerizer/containerizer.cpp 
> fa1d24a56380dd34ed15d3d12e610dfcfe4c56d6 
>   src/slave/containerizer/docker.cpp 7585178f34bdb782f0108d3ab94c23edfacc5564 
>   src/slave/containerizer/fetcher.cpp 
> 8b26e882eced9478ffdc6b2dcbc0a5796c5c3ce2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> cd1e9ae8b8925cb97396dde22b6a3eb63ab65536 
>   src/slave/

Re: Review Request 65504: Made master set `launch_executor` in the RunTask(Group)Message.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/master.cpp
Lines 4920-4924 (original), 4919-4928 (patched)


I think this should be 

```
bool launchExecutor = true;
if (task.has_executor()) {
 launchExecutor = isLaunchExecutor(task.executor().executor_id(), 
framework, slave);
 if (launchExecutor) {
   addExecutor(task.executor(), framework, slave);
   consumed += task.executor().resources()
 }
 
} 

```

otherwise, the log line below says we are launching a command task on an 
existing executor which is wrong.


- Vinod Kone


On Feb. 5, 2018, 2:49 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65504/
> ---
> 
> (Updated Feb. 5, 2018, 2:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
> 
> Also refactored `addTask()` logic. Added two new functions:
> `isTaskLaunchExecutor()` checks if a task needs to launch executor;
> `addExecutor()` adds executor to the framework and slave.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94 
> 
> 
> Diff: https://reviews.apache.org/r/65504/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65571: Handle 'None' passed from the MasterDetector in 'Master::detect()'.

[Vinod Kone]

> On Feb. 8, 2018, 8:22 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 2186 (patched)
> > 
> >
> > s/Leader detector indicated no master elected/No master was elected/
> > 
> > More importantly, this changes the semantics a bit. Previously if this 
> > master was the current leader it committed suicide even in this case. But 
> > we don't anymore. Is that what we want?
> > 
> > 
> > Also, where in the interface does it say that None() is retryable. It 
> > says retryable errors are handled internally by the detector?
> 
> Benno Evers wrote:
> Ok, I guess I misinterpreted the documentation on 
> `MasterDetector::detect()`:
> 
> ```
>* Returns MasterInfo after an election has occurred and the elected
>* master is different than that specified (if any), or NONE if an
>* election occurs and no master is elected (e.g., all masters are
>* lost). A failed future is returned if the detector is unable to
>* detect the leading master due to a non-retryable error.
> ```
> 
> Since electing no master sounds like an error, and the future is not 
> failed, I assumed that this case was implicitly classifid as retryable error.
> 
> Anyways, for the semantics I assume that not aborting is at least what 
> the original author intended, otherwise there would be no point to 
> differentiate between `Error` and `None` in the API.
> 
> Additionally, the code that causes the master to crash in this situation 
> was introduced relatively recently (11 July 2017, a8c7ae44c8), before that 
> the `detected()`-handler would have just set `leader` to `None` and quietly 
> continued. So I would argue that this fix is actually restoring the 
> previously existing behaviour, not changing it.

"...before that the detected()-handler would have just set leader to None and 
quietly continued". Is this true? AFAICT the commit you mentioned only adds 
leader domain related changes, doesn't change the behavior we are talking 
about? See: https://reviews.apache.org/r/59763/


- Vinod


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


On Feb. 9, 2018, 12:55 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65571/
> ---
> 
> (Updated Feb. 9, 2018, 12:55 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Vinod Kone.
> 
> 
> Bugs: MESOS-8550
> https://issues.apache.org/jira/browse/MESOS-8550
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The function `MasterDetector::detect()` returns a value of type
> `Future>`, which, according to its documentation,
> can be `None` if an election occured and no master is elected.
> 
> However, the code in `Master::detected()` was only handling the
> cases of a failed future or a valid `MasterInfo` object.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 
>   src/tests/cluster.hpp ad2b80e658d2f8afcefe9969d62cd33f0c475ce9 
>   src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
> 
> 
> Diff: https://reviews.apache.org/r/65571/diff/2/
> 
> 
> Testing
> ---
> 
> `./mesos-tests`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>