Re: Review Request 65469: Windows: Updated `internal::process:createChildProcess`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65397, 65398, 65399, 65400, 65401, 65402, 65405, 65406, 
65407, 65408, 65409, 65465, 65403, 65467, 65574, 65469]

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. 8, 2018, 7:50 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65469/
> ---
> 
> (Updated Feb. 8, 2018, 7:50 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The interface of `internal::windows:create_process` was changed to
> accept a `std::array` instead of a `std::tuple`.
> 
> Furthermore, the error handling in `subprocess` was incorrect. We only
> need to check the success of `createChildProcess`; we do not need to
> check if `pid == -1` on Windows. We also now return the actual error
> from `create_process` if it errored, instead of the whole
> `Try`.
> 
> The TODO about closing the child-ends of the file descriptors was
> resolved. We now close these in `createChildProcess`, immediately after
> `create_process`. This means we only have to do it once.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 785e2e1083d115d25fffde2df74ed8bc1726511c 
>   3rdparty/libprocess/src/subprocess_windows.hpp 
> 0183bb451f68528acf31ed97754320c64f35102b 
> 
> 
> Diff: https://reviews.apache.org/r/65469/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65571]

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. 8, 2018, 4:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65571/
> ---
> 
> (Updated Feb. 8, 2018, 4:50 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/1/
> 
> 
> Testing
> ---
> 
> `./mesos-tests`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65448: Added a test to ensure master removes executors that never launched.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65448 was successfully built and tested.

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

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

- Mesos Reviewbot Windows


On Feb. 1, 2018, 10:04 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65448/
> ---
> 
> (Updated Feb. 1, 2018, 10:04 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
> ---
> 
> This test ensures that agent send exited executor message when the
> executor is never launched. So that master's executor bookkeeping
> entry is removed. See MESOS-1720.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp b54688add9949b9c4ac2ce3a42503a411e6da09f 
> 
> 
> Diff: https://reviews.apache.org/r/65448/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65465: Windows: Fixed recovery of Mesos containerizer.

[Andrew Schwartzmeyer]

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



Re-ran ctest on Windows, make check on Linux, and manually tested recovery 
scenarios.

- Andrew Schwartzmeyer


On Feb. 8, 2018, 11:54 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65465/
> ---
> 
> (Updated Feb. 8, 2018, 11:54 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8519
> https://issues.apache.org/jira/browse/MESOS-8519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Windows OS deletes the job object created in the agent process when
> the agent dies, because no other process holds a handle to it (despite
> processes being assigned to the job object). While this is
> counter-intuitive, it is the observed behavior. So in order for recovery
> to succeed, the containerizer must also hold an otherwise unused handle
> to its job object to keep it alive in the kernel, and available for
> recovery to find.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 91016ed417428e3a5b21a132a96b9d7760d13aa3 
> 
> 
> Diff: https://reviews.apache.org/r/65465/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> [--] Global test environment tear-down
> [==] 874 tests from 85 test cases ran. (253311 ms total)
> [  PASSED  ] 874 tests.
> 
> I0201 12:46:58.159368  3116 slave.cpp:6921] Recovering framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.159368  3116 slave.cpp:8543] Recovering executor 
> 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.162847  9456 task_status_update_manager.cpp:207] Recovering 
> task status update manager
> I0201 12:46:58.162847  9456 task_status_update_manager.cpp:215] Recovering 
> executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.166851  7344 containerizer.cpp:674] Recovering containerizer
> I0201 12:46:58.167351  7344 containerizer.cpp:731] Recovering container 
> 69cefa53-61e0-444b-a808-e38ffb4cb18f for executor 
> 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:46:58.183379 17088 provisioner.cpp:493] Provisioner recovery complete
> I0201 12:46:58.186367 16792 slave.cpp:6695] Sending reconnect request to 
> executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- at executor(1)@10.123.7.41:52591
> I0201 12:46:58.194370  7344 slave.cpp:4519] Received re-registration message 
> from executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
> I0201 12:47:00.193958 16792 slave.cpp:4737] Cleaning up un-reregistered 
> executors
> I0201 12:47:00.193958 16792 slave.cpp:6824] Finished recovery
> I0201 12:47:00.200943  9456 task_status_update_manager.cpp:181] Pausing 
> sending task status updates
> I0201 12:47:00.200943  3116 slave.cpp:1146] New master detected at 
> master@10.123.6.78:5050
> I0201 12:47:00.200943  3116 slave.cpp:1190] No credentials provided. 
> Attempting to register without authentication
> I0201 12:47:00.200943  3116 slave.cpp:1201] Detecting new master
> I0201 12:47:00.214944 16792 slave.cpp:1471] Re-registered with master 
> master@10.123.6.78:5050
> I0201 12:47:00.214944 13180 task_status_update_manager.cpp:188] Resuming 
> sending task status updates
> I0201 12:47:00.215942 16792 slave.cpp:1516] Forwarding agent update 
> {"operations":{},"resource_version_uuid" 
> {"value":"jLIL1d\/PQnuwmFxpMf8CLQ=="},"slave_id":{"value":"7dc02270-a4e1-4f59-9ad7-56bad5182ea4S3"},"update_oversubscribed_resources":true}
> I0201 12:47:00.219952  3116 slave.cpp:3625] Updating info for framework 
> eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- with pid updated to 
> scheduler-aaa62980-8b1b-4775-b8bb-c6890b41941e@10.123.6.78:45907
> I0201 12:47:00.233942  7344 task_status_update_manager.cpp:188] Resuming 
> sending task status updates
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

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

Relevant logs:

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

```
error: patch failed: site/data/releases.yml:154
error: site/data/releases.yml: patch does not apply
```

- Mesos Reviewbot Windows


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 65580: Improved logging for disk resources.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 9, 2018, 1:09 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65580/
> ---
> 
> (Updated Feb. 9, 2018, 1:09 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For CSI disk resources, the root path is redundunt since it can be
> derived from the source ID, so we print the root path only for non-CSI
> disk resources.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 262dcfb5306fd1824aa8620ccd4b7a14285a7855 
>   src/v1/resources.cpp 06e310f43d1d570bdb4c44dac3640cc1357b54a5 
> 
> 
> Diff: https://reviews.apache.org/r/65580/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Tim Anderegg]

---
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.


Changes
---

Rebased against latest


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


Repository: mesos


Description
---

Support for multiple versions of docs.


Diffs (updated)
-

  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/

Changes: https://reviews.apache.org/r/52064/diff/5-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 65448: Added a test to ensure master removes executors that never launched.

[Vinod Kone]

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


Fix it, then Ship it!





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


s/toLaunchExecutor/launchExecutor/



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


s/exited/Exited/



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


Can you also add a test with default executor? And maybe instead of kill 
task path, try to exercise the authz failure path for that test? Feel free to 
do it in a subsequent review. More tests that exercise the code paths you 
changed the better.


- Vinod Kone


On Feb. 1, 2018, 2:04 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65448/
> ---
> 
> (Updated Feb. 1, 2018, 2:04 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
> ---
> 
> This test ensures that agent send exited executor message when the
> executor is never launched. So that master's executor bookkeeping
> entry is removed. See MESOS-1720.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp b54688add9949b9c4ac2ce3a42503a411e6da09f 
> 
> 
> Diff: https://reviews.apache.org/r/65448/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

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

[Meng Zhu]

> On Feb. 8, 2018, 12:03 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 5185-5186 (patched)
> > 
> >
> > this doesn't seem correct?
> > 
> > shouldn't this be
> > 
> > ```
> > CHECK(_offeredResources.contains(taskResources))
> >  << _offeredResources << " does not contain " << taskResources;
> >  
> > _offeredResources -= taskResources;
> > 
> > ```
> > 
> > also, you need to do another check for executor resources before the 
> > `foreach` loop
> > 
> > ```
> > CHECK(_offeredResources.contains(executorResources))
> >  << _offeredResources << " does not contain " << executorResources;
> >  
> > _offeredResources -= executorResources;
> > 
> > ```

The check is outside of the for loop. `totalResources` is incremented in the 
loop and we are subtracting it all at once outside of the loop.


- Meng


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


On Feb. 4, 2018, 6:49 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65504/
> ---
> 
> (Updated Feb. 4, 2018, 6:49 p.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 65518: Reaped the container process directly in Docker executor.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Feb. 8, 2018, 5:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 8, 2018, 5:03 p.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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65572: Fixed two slave recovery tests.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 9, 2018, 1:12 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65572/
> ---
> 
> (Updated Feb. 9, 2018, 1:12 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-8552
> https://issues.apache.org/jira/browse/MESOS-8552
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two tests are affected by the lingering executor fix
> (#65109). We should wait to make sure tasks are actually launched
> on the executor before trigger the agent failover. Otherwise, the
> container will be shutdown upon re-registration because the executor
> has never received any task.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 
> 
> 
> Diff: https://reviews.apache.org/r/65572/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 65580: Improved logging for disk resources.

[Chun-Hung Hsiao]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

For CSI disk resources, the root path is redundunt since it can be
derived from the source ID, so we print the root path only for non-CSI
disk resources.


Diffs
-

  src/common/resources.cpp 262dcfb5306fd1824aa8620ccd4b7a14285a7855 
  src/v1/resources.cpp 06e310f43d1d570bdb4c44dac3640cc1357b54a5 


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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 65575: Removed redundant comment from the command executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65575]

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. 9, 2018, 3:49 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65575/
> ---
> 
> (Updated Feb. 9, 2018, 3:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed redundant comment from the command executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/65575/diff/1/
> 
> 
> Testing
> ---
> 
> Not a functional change.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65578: Updated documentation for `UriDiskProfileAdaptor`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2018, 11:06 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65578/
> ---
> 
> (Updated Feb. 8, 2018, 11:06 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated documentation for `UriDiskProfileAdaptor`.
> 
> 
> Diffs
> -
> 
>   docs/csi.md ace5bd4c4293747fffe9bc8dbcceaf34ce035f2f 
> 
> 
> Diff: https://reviews.apache.org/r/65578/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65509: Fixed CURL_ prefix in unit test name to succeed on Windows.

[Andrew Schwartzmeyer]

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




src/tests/environment.cpp
Line 219 (original), 219-224 (patched)


Fixing in commit: `#ifndef __WINDOWS__` since `__linux__` isn't our only 
non-Windows platform.


- Andrew Schwartzmeyer


On Feb. 8, 2018, 3:09 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65509/
> ---
> 
> (Updated Feb. 8, 2018, 3:09 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
> ---
> 
> Since the 'curl.exe' executable is built on part of Windows (it's a
> build target), it will always exist for test purposes.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd 
> 
> 
> Diff: https://reviews.apache.org/r/65509/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

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

[Gaston Kleiman]

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




src/tests/health_check_tests.cpp
Lines 109 (patched)


Can you add a comment explaining why we need a custom image?



src/tests/health_check_tests.cpp
Lines 132 (patched)


s/img/image/

or

s/image/pull/


- Gaston Kleiman


On Jan. 30, 2018, 2:21 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64387/
> ---
> 
> (Updated Jan. 30, 2018, 2:21 a.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/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd 
>   src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
>   src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 
> 
> 
> Diff: https://reviews.apache.org/r/64387/diff/10/
> 
> 
> 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 65419: Fixed quoting issues in docker executor command checks.

[Andrew Schwartzmeyer]

> On Feb. 2, 2018, 1:43 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 535-537 (original), 543-547 (patched)
> > 
> >
> > Can you explain why this was changed a little more? Maybe an example 
> > would help.
> 
> Akash Gupta wrote:
> Originally, it was setting the `docker exec` in `shell` mode, which 
> wrapped it with `sh -c ''`. So, the command was `sh -c 'docker exec sh -c " 
>  "'`, which was breaking on Windows due to quoting. Now, it skips the 
> `sh` wrapper and calls `docker exec sh -c ''` (in exec form, so it's 
> actually `["docker", "exec", "sh", "-c", cmd]`) directly.
> 
> This fixes a couple of things:
>  - The tests will actually pass on Windows now. Before, due to the crazy 
> quoting, the tests were failing.
>  - On Linux, your command can now actually have `"` in it instead of 
> having to escape it. For example, `echo "Hello World!"` was originally 
> getting changed to `sh -c 'docker exec sh -c " echo "Hello World!" "'`, which 
> is broken.
>  - On Linux, the mesos-agent shell doesn't accidently interpret the 
> command anymore. For example, if your command was `ls $HOME`, originally, it 
> was running `sh -c 'docker exec sh -c " ls $HOME "'`, which meant that 
> `$HOME` was being   evaluated in the context of the mesos-agent due to 
> the first `sh -c` instead of the docker container.
> 
> I've seen a couple of issues caused due to the weird quoting. ( 
> https://issues.apache.org/jira/browse/MESOS-4812 and 
> https://github.com/mesosphere/marathon/issues/5136)

Hm, take a look at this: https://reviews.apache.org/r/54846/


- Andrew


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


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/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65510: Enabled `curl_uri_fetcher.cpp` tests on Windows platform.

[Jeff Coffler]

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

(Updated Feb. 8, 2018, 11:13 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Summary (updated)
-

Enabled `curl_uri_fetcher.cpp` tests on Windows platform.


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


Repository: mesos


Description (updated)
---

These work now that `curl` is available on Windows.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp 8a40f46b215bb1f267a59a9edfca83445f86b430 


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

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


Testing
---


Thanks,

Jeff Coffler

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

[Gaston Kleiman]

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




src/checks/checker_process.cpp
Lines 1017-1020 (patched)


Why did you introduce a new function? `CheckerProcess::_httpCheck` doesn't 
look like a continuation, since it is called directly (and synchronously) by 
`CheckerProcess::httpCheck`.



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


Add a period at the end of the line.

s/health check/check/


- Gaston Kleiman


On Feb. 8, 2018, 9:51 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 8, 2018, 9:51 a.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/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65508: Allowed curl program to be properly executed on Windows platform.

[Jeff Coffler]

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

(Updated Feb. 8, 2018, 11:09 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Summary (updated)
-

Allowed curl program to be properly executed on Windows platform.


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


Repository: mesos


Description (updated)
---

On Windows, "curl.exe" must be executed rather then "curl".


Diffs (updated)
-

  src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 


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

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


Testing
---

Full build, unit tests on both Linux and Windows platform.


Thanks,

Jeff Coffler

Re: Review Request 65509: Fixed CURL_ prefix in unit test name to succeed on Windows.

[Jeff Coffler]

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

(Updated Feb. 8, 2018, 11:09 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Summary (updated)
-

Fixed CURL_ prefix in unit test name to succeed on Windows.


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


Repository: mesos


Description
---

Since the 'curl.exe' executable is built on part of Windows (it's a
build target), it will always exist for test purposes.


Diffs (updated)
-

  src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd 


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

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


Testing
---


Thanks,

Jeff Coffler

Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.

[Jeff Coffler]

> On Feb. 6, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote:
> > Nit: Fix summary to be in past tense, and fix description to not be a copy 
> > of the summary.
> 
> Jeff Coffler wrote:
> This was a one-line commit, but I went ahead and added a second line. I'm 
> not sure why you said that the description shouldn't be a copy of the 
> summary, it wasn't. Tooling problem on your side, or just a mistake?
> 
> Andrew Schwartzmeyer wrote:
> When you post a review with just a summary, the scripts copy the summary 
> into the description. This unfortunately carries through to the other side 
> when the review is applied, and then you have a weird commit. The 
> (work-around?) is to not post one-line commit messages.
> 
> Jeff Coffler wrote:
> That's odd. I think you mentioned this before, but I see one-line commits 
> on Mesos, like this:
> 
> ```
> commit 2ef5f4a
> Author: Gilbert Song 
> Date:   Thu Feb 8 08:45:42 2018
> 
> Added an image 1.3-1.5_v1_json_state_query_latency.png to docs/images.
> 
> commit f7dbd29
> Author: Michael Park 
> Date:   Wed Feb 7 20:28:42 2018
> 
> Updated ReviewBot to use Ubuntu 16.04.
> ```
> 
> Am I doing something wrong. Do I need something "funny" in the 
> description (like a "." or something) if it's one line?

Talked F2F with Andy. This is a tooling issue. One-line commits are possible, 
if after rbt, the copy of the one-line commit is removed from the "Description" 
field.


- Jeff


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


On Feb. 5, 2018, 6:12 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65508/
> ---
> 
> (Updated Feb. 5, 2018, 6:12 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
> ---
> 
> Allow curl program to be properly executed on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 
> 
> 
> Diff: https://reviews.apache.org/r/65508/diff/1/
> 
> 
> Testing
> ---
> 
> Full build, unit tests on both Linux and Windows platform.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Review Request 65578: Updated documentation for `UriDiskProfileAdaptor`.

[Chun-Hung Hsiao]

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

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


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


Repository: mesos


Description
---

Updated documentation for `UriDiskProfileAdaptor`.


Diffs
-

  docs/csi.md ace5bd4c4293747fffe9bc8dbcceaf34ce035f2f 


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


Testing
---

N/A


Thanks,

Chun-Hung Hsiao

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

[Gaston Kleiman]

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




src/checks/checker_process.cpp
Lines 97-100 (original), 100-103 (patched)


These are unused now:

```
  CXX  checks/libmesos_no_3rdparty_la-health_checker.lo
../../src/checks/checker_process.cpp:102:16: error: unused variable 
'DEFAULT_IPV4_DOMAIN' [-Werror,-Wunused-const-variable]
constexpr char DEFAULT_IPV4_DOMAIN[] = "127.0.0.1";
   ^
../../src/checks/checker_process.cpp:103:16: error: unused variable 
'DEFAULT_IPV6_DOMAIN' [-Werror,-Wunused-const-variable]
constexpr char DEFAULT_IPV6_DOMAIN[] = "::1";
   ^
2 errors generated.
```


- Gaston Kleiman


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



PASS: Mesos patch 65497 was successfully built and tested.

Reviews applied: `['65497']`

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

- Mesos Reviewbot Windows


On Feb. 3, 2018, 1:18 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 3, 2018, 1:18 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/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 
>   src/tests/slave_tests.cpp b54688add9949b9c4ac2ce3a42503a411e6da09f 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

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

[Gaston Kleiman]

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



Thanks for this refactoring, the code looks much better and is way easier to 
understand this way.


src/checks/checker_process.hpp
Lines 53 (patched)


nit: s/this field/the `ipv6` parameter/



src/checks/checker_process.cpp
Lines 188-200 (original), 223-230 (patched)


I'd remove the leading underscores in `_checkInfo`, `_launcherDir`, 
`_scheme`, since there is no shadowing to prevent.



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


Nit: add a period at the end of the line.



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


There's no "A" in the table.

s/calling/executing/



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


s/isolated/entered/



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


s/DOCKER/Docker/



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


s/namespace/namespaces/



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


s/namespace/namespaces/



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


I'd write:

`Use Agent API calls to run the command as a nested container.`



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.



src/checks/checker_process.cpp
Lines 484-485 (patched)


What about removing this line and passing the result of 
`genCloneFun(plain)` directly to `process:subprocess`?

i.e.,

```
s = process::subprocess(
command.value(),
Subprocess::PATH(os::DEV_NULL),
Subprocess::FD(STDERR_FILENO),
Subprocess::FD(STDERR_FILENO),
environment,
genCloneFunc(plain));
```



src/checks/checker_process.cpp
Lines 978-979 (patched)


ditto removing this line.


- Gaston Kleiman


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 65566: Returned profiles based on provider selectors in UriDiskProfileAdaptor.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2018, 6:39 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65566/
> ---
> 
> (Updated Feb. 8, 2018, 6:39 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the URI disk profile adaptor module will return the set of profiles
> in which each profile is either known to a storage resource provider or
> applies to it (based on the resource provider selector) when it watches
> for profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/uri_disk_profile.hpp 
> 2f4fc7cededeb41afe329e1315569e097b1a60f9 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 7372b290d4fc3e45dc83fb3dd759150d08b7541f 
> 
> 
> Diff: https://reviews.apache.org/r/65566/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65403: Windows: Disabled `O_CLOEXEC` semantic mapping on Windows.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 2:36 p.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Changes
---

Re-arrange commits.


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


Repository: mesos


Description
---

Previously we mapped `O_CLOEXEC` to `O_NOINHERIT`, which is close to the
intended semantics, but not close enough. Instead, we want all Windows
handles by default to be non-inheritable, and not tie any Windows
inheritance semantics to the close-on-exec semantics. So, we define
`O_CLOEXEC` on Windows to be `0`, and remove the logging from the
`os::cloexec` family of functions because do not ever intend to
implement them.

We achieve the "close" part of the semantics by making all handles
non-inheritable, as the intent is to avoid leaking handles.


Diffs
-

  3rdparty/stout/include/stout/os/open.hpp 
6711f22d44b0e7c13da3abd51f9fb7b71779e868 
  3rdparty/stout/include/stout/os/windows/fcntl.hpp 
5800ec92f85401a80cb813afd880be2e5a24a3af 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65405: Implemented `net::socket()` for Windows using `WSASocket`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 2:36 p.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Changes
---

Re-arrange commits.


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


Repository: mesos


Description
---

Rather than reusing the existing `net::socket()` wrapper for Windows,
the original wrapper was moved to `posix/socket.hpp`, and a new wrapper
was written in `windows/socket.hpp` which uses `::WSASocket()` instead
of `::socket()` and takes an optional set of Windows socket flags.

We enable `WSA_FLAG_OVERLAPPED` so that the socket supports overlapped
I/O operations (the default behavior of `::socket()`; MSDN states that
most sockets should be created with this flag set, so it is a reasonable
default.

Furthermore, we enable `WSA_FLAG_NO_HANDLE_INHERIT`, which provides the
semantics of close-on-exec. This is set by default because (a) it is not
possible to change afterwards, and (b) we never use inheritable sockets.
Therefore it reasonable to also disable inheritance.


Diffs
-

  3rdparty/stout/include/stout/os/posix/socket.hpp 
bab0b808f53abd1314a7d13fc0cba75e5717f96f 
  3rdparty/stout/include/stout/os/socket.hpp 
2ca4465ca23c9ca59239947c9babf8dd0212fafd 
  3rdparty/stout/include/stout/os/windows/socket.hpp 
020c5e2367217cd2b4439ae5e2d5be4b31b4226b 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65467: Windows: Added `internal::windows::set_inherit(WindowsFD, bool)`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 2:35 p.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Changes
---

Re-arrange commits.


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


Repository: mesos


Description
---

This function is used on Windows to explicitly enable or disable
inheritance on a handle.


Diffs
-

  3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.

[Jeff Coffler]

> On Feb. 6, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote:
> > Nit: Fix summary to be in past tense, and fix description to not be a copy 
> > of the summary.
> 
> Jeff Coffler wrote:
> This was a one-line commit, but I went ahead and added a second line. I'm 
> not sure why you said that the description shouldn't be a copy of the 
> summary, it wasn't. Tooling problem on your side, or just a mistake?
> 
> Andrew Schwartzmeyer wrote:
> When you post a review with just a summary, the scripts copy the summary 
> into the description. This unfortunately carries through to the other side 
> when the review is applied, and then you have a weird commit. The 
> (work-around?) is to not post one-line commit messages.

That's odd. I think you mentioned this before, but I see one-line commits on 
Mesos, like this:

```
commit 2ef5f4a
Author: Gilbert Song 
Date:   Thu Feb 8 08:45:42 2018

Added an image 1.3-1.5_v1_json_state_query_latency.png to docs/images.

commit f7dbd29
Author: Michael Park 
Date:   Wed Feb 7 20:28:42 2018

Updated ReviewBot to use Ubuntu 16.04.
```

Am I doing something wrong. Do I need something "funny" in the description 
(like a "." or something) if it's one line?


- Jeff


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


On Feb. 5, 2018, 6:12 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65508/
> ---
> 
> (Updated Feb. 5, 2018, 6:12 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
> ---
> 
> Allow curl program to be properly executed on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 
> 
> 
> Diff: https://reviews.apache.org/r/65508/diff/1/
> 
> 
> Testing
> ---
> 
> Full build, unit tests on both Linux and Windows platform.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.

[Andrew Schwartzmeyer]

> On Feb. 6, 2018, 11:12 a.m., Andrew Schwartzmeyer wrote:
> > Nit: Fix summary to be in past tense, and fix description to not be a copy 
> > of the summary.
> 
> Jeff Coffler wrote:
> This was a one-line commit, but I went ahead and added a second line. I'm 
> not sure why you said that the description shouldn't be a copy of the 
> summary, it wasn't. Tooling problem on your side, or just a mistake?

When you post a review with just a summary, the scripts copy the summary into 
the description. This unfortunately carries through to the other side when the 
review is applied, and then you have a weird commit. The (work-around?) is to 
not post one-line commit messages.


- Andrew


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


On Feb. 5, 2018, 10:12 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65508/
> ---
> 
> (Updated Feb. 5, 2018, 10:12 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
> ---
> 
> Allow curl program to be properly executed on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 
> 
> 
> Diff: https://reviews.apache.org/r/65508/diff/1/
> 
> 
> Testing
> ---
> 
> Full build, unit tests on both Linux and Windows platform.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 65553: Passed `ResourceProviderInfo` to disk profile adaptor modules.

[Chun-Hung Hsiao]

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

(Updated Feb. 8, 2018, 10:22 p.m.)


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


Changes
---

Addressed Jie's comment.


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


Repository: mesos


Description
---

Passed `ResourceProviderInfo` to disk profile adaptor modules.


Diffs (updated)
-

  include/mesos/resource_provider/storage/disk_profile.hpp 
d1a522ad23a7d8a53d5c57e9d4191e1e7652f0c8 
  src/resource_provider/storage/disk_profile.cpp 
053fb7ac0857cbd1c57e3d75f1445ac6f083852b 
  src/resource_provider/storage/provider.cpp 
786a0cecd66e07556434a8e00712ffd5f31189b2 
  src/resource_provider/storage/uri_disk_profile.hpp 
2f4fc7cededeb41afe329e1315569e097b1a60f9 
  src/resource_provider/storage/uri_disk_profile.cpp 
7372b290d4fc3e45dc83fb3dd759150d08b7541f 


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

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


Testing
---

Tests done in the next patch in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.

[Jeff Coffler]

> On Feb. 6, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote:
> > Nit: Fix summary to be in past tense, and fix description to not be a copy 
> > of the summary.

This was a one-line commit, but I went ahead and added a second line. I'm not 
sure why you said that the description shouldn't be a copy of the summary, it 
wasn't. Tooling problem on your side, or just a mistake?


- Jeff


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


On Feb. 5, 2018, 6:12 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65508/
> ---
> 
> (Updated Feb. 5, 2018, 6:12 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
> ---
> 
> Allow curl program to be properly executed on Windows platform.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd 
> 
> 
> Diff: https://reviews.apache.org/r/65508/diff/1/
> 
> 
> Testing
> ---
> 
> Full build, unit tests on both Linux and Windows platform.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

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?

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.


> 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?

Why? The agent failover happens after the executor has been registered. Then 
the executor re-registers and got killed.


> 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.

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".


> On Feb. 7, 2018, 6:19 p.m., Benjamin Mahler wrote:
> > src/tests/slave_tests.cpp
> > Lines 5032-5038 (patched)
> > 
> >
> > Instead of using a slave mock here, can we just expect the executorLost 
> > signal from the scheduler?

This is v1 scheduler, I think the `Failure` message is not as clear as the 
`_shutdownExecutor` call of the slave.


- Meng


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


On Feb. 2, 2018, 5:18 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 2, 2018, 5:18 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/mock_slave.cpp 597d7abef20dd5f89b16e4616233f02760b9d037 
>   src/tests/slave_tests.cpp b54688add9949b9c4ac2ce3a42503a411e6da09f 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65559: Updated tests for resource provider selector support.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2018, 6:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65559/
> ---
> 
> (Updated Feb. 8, 2018, 6:38 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated tests for resource provider selector support.
> 
> 
> Diffs
> -
> 
>   src/tests/disk_profile_tests.cpp b6b35ef3641b2a5b59ee2190223e1e75ef3abaaf 
>   src/tests/storage_local_resource_provider_tests.cpp 
> f3903089f626a0ac72268424699f642e2df32e6c 
> 
> 
> Diff: https://reviews.apache.org/r/65559/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> NOTE: make check will fail for r65558 without this patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65558: Added helpers for supporting resource provider selectors.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2018, 4:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65558/
> ---
> 
> (Updated Feb. 8, 2018, 4:37 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patches validates that a profile's manifest must have a selector
> and adds a helper function for selecting resource providers.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> fa2c7ec5055577b12e5e795064bf848b9a887def 
>   src/resource_provider/storage/disk_profile_utils.cpp 
> c84b6e18665086968808fb5cb0a313c1050ea587 
> 
> 
> Diff: https://reviews.apache.org/r/65558/diff/3/
> 
> 
> Testing
> ---
> 
> Tests done later in chain.
> 
> NOTE: The next patch in chain only include necessary changes for the 
> validation tests. Tests for the helper function will be added in a separated 
> patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65538: Added resource provider selectors in `disk_profile.proto`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2018, 4:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65538/
> ---
> 
> (Updated Feb. 8, 2018, 4:36 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds two selectors to the `CSIManifest` protobuf so that the
> URI disk profile adaptor can be customized to notify each resource
> provider with a different set of profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/disk_profile.proto 
> 6cf1f8abcd24e45a292efc95a395f90bb2140da2 
> 
> 
> Diff: https://reviews.apache.org/r/65538/diff/6/
> 
> 
> Testing
> ---
> 
> Tests done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65554: Updated disk profile tests due to changes in the module interface.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 7, 2018, 9:05 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65554/
> ---
> 
> (Updated Feb. 7, 2018, 9:05 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated disk profile tests due to changes in the module interface.
> 
> 
> Diffs
> -
> 
>   src/tests/disk_profile_tests.cpp b6b35ef3641b2a5b59ee2190223e1e75ef3abaaf 
> 
> 
> Diff: https://reviews.apache.org/r/65554/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> NOTE: make check will fail for r65553 without this patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65553: Passed `ResourceProviderInfo` to disk profile adaptor modules.

[Jie Yu]

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


Fix it, then Ship it!





include/mesos/resource_provider/storage/disk_profile.hpp
Lines 24 (patched)


Can you move this down after csi/spec.hpp? otherwise, you should move 
csi/spec.hpp up.


- Jie Yu


On Feb. 7, 2018, 9:04 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65553/
> ---
> 
> (Updated Feb. 7, 2018, 9:04 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8510
> https://issues.apache.org/jira/browse/MESOS-8510
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passed `ResourceProviderInfo` to disk profile adaptor modules.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/storage/disk_profile.hpp 
> d1a522ad23a7d8a53d5c57e9d4191e1e7652f0c8 
>   src/resource_provider/storage/disk_profile.cpp 
> 053fb7ac0857cbd1c57e3d75f1445ac6f083852b 
>   src/resource_provider/storage/provider.cpp 
> 604cadff20a6156639db10a28a180e53a959ff29 
>   src/resource_provider/storage/uri_disk_profile.hpp 
> 2f4fc7cededeb41afe329e1315569e097b1a60f9 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 7372b290d4fc3e45dc83fb3dd759150d08b7541f 
> 
> 
> Diff: https://reviews.apache.org/r/65553/diff/1/
> 
> 
> Testing
> ---
> 
> Tests done in the next patch in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65469: Windows: Updated `internal::process:createChildProcess`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65469 was successfully built and tested.

Reviews applied: `['65399', '65400', '65401', '65402', '65403', '65405', 
'65406', '65407', '65408', '65409', '65465', '65467', '65574', '65469']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 7:50 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65469/
> ---
> 
> (Updated Feb. 8, 2018, 7:50 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The interface of `internal::windows:create_process` was changed to
> accept a `std::array` instead of a `std::tuple`.
> 
> Furthermore, the error handling in `subprocess` was incorrect. We only
> need to check the success of `createChildProcess`; we do not need to
> check if `pid == -1` on Windows. We also now return the actual error
> from `create_process` if it errored, instead of the whole
> `Try`.
> 
> The TODO about closing the child-ends of the file descriptors was
> resolved. We now close these in `createChildProcess`, immediately after
> `create_process`. This means we only have to do it once.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 785e2e1083d115d25fffde2df74ed8bc1726511c 
>   3rdparty/libprocess/src/subprocess_windows.hpp 
> 0183bb451f68528acf31ed97754320c64f35102b 
> 
> 
> Diff: https://reviews.apache.org/r/65469/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65575: Removed redundant comment from the command executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65575 was successfully built and tested.

Reviews applied: `['65575']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 7:49 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65575/
> ---
> 
> (Updated Feb. 8, 2018, 7:49 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, Qian Zhang, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed redundant comment from the command executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/65575/diff/1/
> 
> 
> Testing
> ---
> 
> Not a functional change.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

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

[Vinod Kone]

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



Description: s/linger/lingers/


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


`None()` on next line please



src/slave/slave.cpp
Line 2104 (original), 2127 (patched)


don't need to send here?



src/slave/slave.cpp
Line 2543 (original), 2626 (patched)


TASK_LOST or ExitedExecutorMessage



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


for a command task `executor` should be null. Can you do a CHECK instead?



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


s/We will drop tasks/In this case we will drop tasks/



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


should we send this only if executor state is TERMINATED? If the executor 
is in any other state, we are shutting it down below which should result in a 
ExitedExecutorMessage?

Also, can you add a TODO to have master do proper reconciliation instead of 
removing the executor entry in this case?



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


s/And the master will no longer contain/Master then removes/


- 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 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
>   src/tests/mock_slave.hpp 29ce7140501888d95d5f2d6c26b752ad276b484a 
>   src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65449/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated test in #65448
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 59989: Added a test `ProtobufTest.JsonifyMap`.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Dec. 20, 2017, 1:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59989/
> ---
> 
> (Updated Dec. 20, 2017, 1:03 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ProtobufTest.JsonifyMap`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> be35ad0d1e16501df67752a1818f79751419a43d 
> 
> 
> Diff: https://reviews.apache.org/r/59989/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59988: Added a new protobuf message `MapMessage` for protobuf tests.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Dec. 20, 2017, 1:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> ---
> 
> (Updated Dec. 20, 2017, 1:04 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new protobuf message `MapMessage` for protobuf tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.proto 
> cf8aadc7c003b3165421edb5ff2e71b6abf526f4 
> 
> 
> Diff: https://reviews.apache.org/r/59988/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

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



Overall good. Thanks for your efforts!


3rdparty/stout/include/stout/protobuf.hpp
Lines 499-500 (patched)


How about using `UNREACHABLE` instead?



3rdparty/stout/include/stout/protobuf.hpp
Lines 502-503 (patched)


Deprecated or not, it cannot be the type of a key, so let's merge this case 
with the above cases.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1045-1046 (patched)


Ditto.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1045-1051 (patched)


Ditto.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1113-1114 (patched)


Since stout is positioned as a standalone library, we should return an 
error indicating that this is not supported instead of arbitrarily terminate 
the program.



3rdparty/stout/include/stout/protobuf.hpp
Lines 942-943 (original), 1180-1181 (patched)


Not yours, but returning an error seems better for the same argument above.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1005-1006 (original), 1243-1244 (patched)


Ditto.


- Chun-Hung Hsiao


On Dec. 21, 2017, 2:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Dec. 21, 2017, 2:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65446: Added helper function for the agent to send `ExitedExecutorMessage`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 5, 2018, 2:45 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65446/
> ---
> 
> (Updated Feb. 5, 2018, 2:45 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
> ---
> 
> Added helper function for the agent to send `ExitedExecutorMessage`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65446/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

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

[Vinod Kone]

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




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?


- Vinod Kone


On Feb. 8, 2018, 4:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65571/
> ---
> 
> (Updated Feb. 8, 2018, 4:50 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/1/
> 
> 
> Testing
> ---
> 
> `./mesos-tests`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > 
> >
> > How about a partial specialization for `google::protobuf::Map`?
> 
> Qian Zhang wrote:
> Why do we need to do that? What do we want to achieve with such a partial 
> specialization?
> 
> Chun-Hung Hsiao wrote:
> For providing the `parse>(const JSON::Value& 
> value)` function. Say if we want to create a protobuf message `m` that 
> contains a `map map_field`, we cannot just do 
> `m.mutable_map_field()->CopyFrom(parse(json))`.
>  Instead we probably need something like 
> `m.mutable_map_field()->swap(parse 
> Chun-Hung Hsiao wrote:
> Oh we probably cannot just apply `swap` on an r-value. But you know want 
> I mean ;)
> 
> Qian Zhang wrote:
> So you mean just like the partial specialization for 
> `google::protobuf::RepeatedPtrField`:
> ```
> template 
> struct Parse
> ```
> We can add a partial specialization for `google::protobuf::Map`, right? I 
> see there are a lot of places in our code to call the partial specialization 
> for `google::protobuf::RepeatedPtrField`, but I am not sure if we have a need 
> to call the partial specialization for `google::protobuf::Map` in our code.

Consider stout can be standalone it would be nice to have it for completeness. 
But let's do it later when needed. Dropping this issue for now.


- Chun-Hung


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


On Dec. 21, 2017, 2:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Dec. 21, 2017, 2:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65573: Fixed an error in the 1.3.2 CHANGELOG.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65573 was successfully built and tested.

Reviews applied: `['65553', '65554', '65538', '65558', '65559', '65566', 
'65573']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 11:12 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65573/
> ---
> 
> (Updated Feb. 8, 2018, 11:12 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an error in the 1.3.2 CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG b7c38f54d9f728d13c319daa4e43c658118ff1b7 
> 
> 
> Diff: https://reviews.apache.org/r/65573/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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




src/master/master.cpp
Line 3832 (original), 3832 (patched)


Can you add a comment here that for command tasks, even though an executor 
is launched by the agent master is oblivious to that fact? Alternatively, this 
should return true for command tasks.

My proposal:

```
bool islaunchExecutor(
   const ExecutorID& executorId,
   Framework* framework,
   Slave* slave) {

   CHECK_NOTNULL(framework);
   CHECK_NOTNULL(slave);
   
   if (!slave->hasExecutor(framework->id(), executorId())) {
  CHECK(!framework->hasExecutor(slave->id, executorId()))
<< "Executor '" << executorId
<< "' known to the framework " << *framework
<< " but unknown to the agent " << *slave;

  return true;
}
  }
  
  return false;
}
```



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


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



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


s/message.launch_executor()/launchExecutor/



src/master/master.cpp
Lines 5154-5165 (patched)


```
bool launchExecutor = isLaunchExecutor(executor.executor_id(), framework, 
slave);
if (launchExecutor) {
  addExecutor(executor, framework, slave);
  executorResources = executor.resources();
  totalResources += executorResources;
}

```



src/master/master.cpp
Lines 5185-5186 (patched)


this doesn't seem correct?

shouldn't this be

```
CHECK(_offeredResources.contains(taskResources))
 << _offeredResources << " does not contain " << taskResources;
 
_offeredResources -= taskResources;

```

also, you need to do another check for executor resources before the 
`foreach` loop

```
CHECK(_offeredResources.contains(executorResources))
 << _offeredResources << " does not contain " << executorResources;
 
_offeredResources -= executorResources;

```



src/master/master.cpp
Line 5159 (original), 5201 (patched)


if we do the changes above maybe we can just have `taskGroupResources` 
variable instead of `totalResources` and use it here?



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


s/message.launch_executor()/executor/


- 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 cc2685a6bc14103c639ce776cf1c912361e93381 
> 
> 
> Diff: https://reviews.apache.org/r/65504/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 65574: Windows: Fixed handle inheritance in `create_process` wrapper.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Repository: mesos


Description
---

The primary change in this patch is that `create_process` now enables
inheritance for the `pipe` handles passed before starting the child
process. This is required, otherwise the child process will behave
incorrectly (for example, it will write to `stdout` but that will go
nowhere, as the redirection silently failed). After the process is
created, inheritance is disabled to prevent further calls to
`create_process` from inheriting the wrong handles.

The `std::tuple` type was changed to a
`std::array` as it is significantly easier to work
with (it supports iteration).


Diffs
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
542039c31f94eda1af121335b12edf9b83725ae5 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65465: Windows: Fixed recovery of Mesos containerizer.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:54 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

The Windows OS deletes the job object created in the agent process when
the agent dies, because no other process holds a handle to it (despite
processes being assigned to the job object). While this is
counter-intuitive, it is the observed behavior. So in order for recovery
to succeed, the containerizer must also hold an otherwise unused handle
to its job object to keep it alive in the kernel, and available for
recovery to find.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 
91016ed417428e3a5b21a132a96b9d7760d13aa3 


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

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


Testing
---

```
[--] Global test environment tear-down
[==] 874 tests from 85 test cases ran. (253311 ms total)
[  PASSED  ] 874 tests.

I0201 12:46:58.159368  3116 slave.cpp:6921] Recovering framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0201 12:46:58.159368  3116 slave.cpp:8543] Recovering executor 
'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0201 12:46:58.162847  9456 task_status_update_manager.cpp:207] Recovering task 
status update manager
I0201 12:46:58.162847  9456 task_status_update_manager.cpp:215] Recovering 
executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0201 12:46:58.166851  7344 containerizer.cpp:674] Recovering containerizer
I0201 12:46:58.167351  7344 containerizer.cpp:731] Recovering container 
69cefa53-61e0-444b-a808-e38ffb4cb18f for executor 
'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0201 12:46:58.183379 17088 provisioner.cpp:493] Provisioner recovery complete
I0201 12:46:58.186367 16792 slave.cpp:6695] Sending reconnect request to 
executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- at executor(1)@10.123.7.41:52591
I0201 12:46:58.194370  7344 slave.cpp:4519] Received re-registration message 
from executor 'notepad.01d79d48-0791-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0201 12:47:00.193958 16792 slave.cpp:4737] Cleaning up un-reregistered 
executors
I0201 12:47:00.193958 16792 slave.cpp:6824] Finished recovery
I0201 12:47:00.200943  9456 task_status_update_manager.cpp:181] Pausing sending 
task status updates
I0201 12:47:00.200943  3116 slave.cpp:1146] New master detected at 
master@10.123.6.78:5050
I0201 12:47:00.200943  3116 slave.cpp:1190] No credentials provided. Attempting 
to register without authentication
I0201 12:47:00.200943  3116 slave.cpp:1201] Detecting new master
I0201 12:47:00.214944 16792 slave.cpp:1471] Re-registered with master 
master@10.123.6.78:5050
I0201 12:47:00.214944 13180 task_status_update_manager.cpp:188] Resuming 
sending task status updates
I0201 12:47:00.215942 16792 slave.cpp:1516] Forwarding agent update 
{"operations":{},"resource_version_uuid" 
{"value":"jLIL1d\/PQnuwmFxpMf8CLQ=="},"slave_id":{"value":"7dc02270-a4e1-4f59-9ad7-56bad5182ea4S3"},"update_oversubscribed_resources":true}
I0201 12:47:00.219952  3116 slave.cpp:3625] Updating info for framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- with pid updated to 
scheduler-aaa62980-8b1b-4775-b8bb-c6890b41941e@10.123.6.78:45907
I0201 12:47:00.233942  7344 task_status_update_manager.cpp:188] Resuming 
sending task status updates
```


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65408: Windows: Ported `slave_recovery_tests.cpp`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:53 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

This commit enables the unit tests found in `slave_recovery_tests.cpp`
to test agent recovery on Windows.


Diffs (updated)
-

  src/tests/CMakeLists.txt 6f1c67a6b4003ada1a0635a68fb7471895a2a6f5 
  src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 


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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65409: Fixed `SlaveRecoveryTest.ReconcileTasksMissingFromSlave`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:53 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

Because it is not possible to delete a file (or a folder recursively)
with open handles on Windows, we have to explicitly `reset()` the agent
before removing the framework meta directory. Otherwise, the task status
update manager will be destructed too late, and so an open handle for
`task.updates` will cause the `os::rmdir` to fail.

This is safe because we previously destructed the agent anyway, just
later in the test when it was reassigned.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 


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

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


Testing
---

make check on CentOS 7, all passed
ctest on Windows, all passed including new SlaveRecoveryTests

Note that while this chain enables recovery of Docker tasks on Windows, it 
explicitly does not fix MESOS-8519 (recovery of job object tasks).

```
I0131 11:52:01.545505  8316 docker.cpp:898] Recovering Docker containers
I0131 11:52:01.546005   660 containerizer.cpp:674] Recovering containerizer
I0131 11:52:01.546505   660 containerizer.cpp:725] Skipping recovery of 
executor 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- because it was not launched from 
mesos containerizer
I0131 11:52:01.557006 11272 provisioner.cpp:493] Provisioner recovery complete
I0131 11:52:02.521003  8720 docker.cpp:1008] Recovering container 
'f7978e90-32f5-458d-ad4e-3ffa25a7b190' for executor 
'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0131 11:52:02.530527  8316 slave.cpp:6695] Sending reconnect request to 
executor 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- at executor(1)@10.123.7.41:63903
I0131 11:52:02.549062  8720 slave.cpp:4519] Received re-registration message 
from executor 'iis.feae9d12-06ba-11e8-8f77-02421c3bc93c' of framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf-
I0131 11:52:04.548064 10556 slave.cpp:4737] Cleaning up un-reregistered 
executors
I0131 11:52:04.548064 10556 slave.cpp:6824] Finished recovery
I0131 11:52:04.566066   660 task_status_update_manager.cpp:181] Pausing sending 
task status updates
I0131 11:52:04.567059 14636 slave.cpp:1146] New master detected at 
master@10.123.6.78:5050
I0131 11:52:04.567059 14636 slave.cpp:1190] No credentials provided. Attempting 
to register without authentication
I0131 11:52:04.568047 14636 slave.cpp:1201] Detecting new master
I0131 11:52:04.604035  8720 slave.cpp:1471] Re-registered with master 
master@10.123.6.78:5050
I0131 11:52:04.605060   660 task_status_update_manager.cpp:188] Resuming 
sending task status updates
I0131 11:52:04.606036  8720 slave.cpp:1516] Forwarding agent update 
{"operations":{},"resource_version_uuid":{"value":"mzwol7M6SrGxOml4zYlA8Q=="},"slave_id":{"value":"7dc02270-a4e1-4f59-9ad7-56bad5182ea4-S0"},"update_oversubscribed_resource
s":true}
I0131 11:52:04.612036  8720 slave.cpp:3625] Updating info for framework 
eb32cef4-c503-4ab7-85d4-8d4577e6a3bf- with pid updated to 
scheduler-aaa62980-8b1b-4775-b8bb-c6890b41941e@10.123.6.78:45907
I0131 11:52:04.636543 13468 task_status_update_manager.cpp:188] Resuming 
sending task status updates
```


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65407: Windows: Enabled tests that were blocked by MESOS-7604.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:52 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This commit enables tests that were previously disabled on Windows due
to MESOS-7604.


Diffs (updated)
-

  src/tests/api_tests.cpp 43517b5dd8b32a9b824baa653be5e3dca900f26e 
  src/tests/gc_tests.cpp 0e34c017f2c629b527915218f517b88d8392aa4b 
  src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
  src/tests/slave_tests.cpp b54688add9949b9c4ac2ce3a42503a411e6da09f 


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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65405: Implemented `net::socket()` for Windows using `WSASocket`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:51 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

Rather than reusing the existing `net::socket()` wrapper for Windows,
the original wrapper was moved to `posix/socket.hpp`, and a new wrapper
was written in `windows/socket.hpp` which uses `::WSASocket()` instead
of `::socket()` and takes an optional set of Windows socket flags.

We enable `WSA_FLAG_OVERLAPPED` so that the socket supports overlapped
I/O operations (the default behavior of `::socket()`; MSDN states that
most sockets should be created with this flag set, so it is a reasonable
default.

Furthermore, we enable `WSA_FLAG_NO_HANDLE_INHERIT`, which provides the
semantics of close-on-exec. This is set by default because (a) it is not
possible to change afterwards, and (b) we never use inheritable sockets.
Therefore it reasonable to also disable inheritance.


Diffs
-

  3rdparty/stout/include/stout/os/posix/socket.hpp 
bab0b808f53abd1314a7d13fc0cba75e5717f96f 
  3rdparty/stout/include/stout/os/socket.hpp 
2ca4465ca23c9ca59239947c9babf8dd0212fafd 
  3rdparty/stout/include/stout/os/windows/socket.hpp 
020c5e2367217cd2b4439ae5e2d5be4b31b4226b 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65467: Windows: Added `internal::windows::set_inherit(WindowsFD, bool)`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:49 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Changes
---

Make it `set(bool)` so it can be used to turn things on and off.


Summary (updated)
-

Windows: Added `internal::windows::set_inherit(WindowsFD, bool)`.


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


Repository: mesos


Description (updated)
---

This function is used on Windows to explicitly enable or disable
inheritance on a handle.


Diffs (updated)
-

  3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION 


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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65469: Windows: Updated `internal::process:createChildProcess`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2018, 11:50 a.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


Changes
---

Pretty much a new commit.


Summary (updated)
-

Windows: Updated `internal::process:createChildProcess`.


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


Repository: mesos


Description (updated)
---

The interface of `internal::windows:create_process` was changed to
accept a `std::array` instead of a `std::tuple`.

Furthermore, the error handling in `subprocess` was incorrect. We only
need to check the success of `createChildProcess`; we do not need to
check if `pid == -1` on Windows. We also now return the actual error
from `create_process` if it errored, instead of the whole
`Try`.

The TODO about closing the child-ends of the file descriptors was
resolved. We now close these in `createChildProcess`, immediately after
`create_process`. This means we only have to do it once.


Diffs (updated)
-

  3rdparty/libprocess/src/subprocess.cpp 
785e2e1083d115d25fffde2df74ed8bc1726511c 
  3rdparty/libprocess/src/subprocess_windows.hpp 
0183bb451f68528acf31ed97754320c64f35102b 


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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 65575: Removed redundant comment from the command executor.

[Gaston Kleiman]

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

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


Repository: mesos


Description
---

Removed redundant comment from the command executor.


Diffs
-

  src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 


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


Testing
---

Not a functional change.


Thanks,

Gaston Kleiman

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

[Akash Gupta]

> On Feb. 7, 2018, 1:18 p.m., Alexander Rukletsov wrote:
> > src/checks/checks_runtime.hpp
> > Lines 55-57 (patched)
> > 
> >
> > This is not quite true: you only mean command health checks. HTTP and 
> > TCP are still performed by the executor directly.

Yeah that's true. I'll fix up the comment.


- Akash


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


On Feb. 8, 2018, 7:44 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65394/
> ---
> 
> (Updated Feb. 8, 2018, 7:44 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> 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. 8, 2018, 7:44 p.m.)


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


Changes
---

rebased and addressed feedback


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/3/

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


Testing
---

make check


Thanks,

Akash Gupta

Re: Review Request 65573: Fixed an error in the 1.3.2 CHANGELOG.

[Vinod Kone]

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


Ship it!




Doesn't need to depend on the earlier chain?

- Vinod Kone


On Feb. 8, 2018, 7:12 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65573/
> ---
> 
> (Updated Feb. 8, 2018, 7:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an error in the 1.3.2 CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG b7c38f54d9f728d13c319daa4e43c658118ff1b7 
> 
> 
> Diff: https://reviews.apache.org/r/65573/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65572: Fixed two slave recovery tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65572 was successfully built and tested.

Reviews applied: `['65572']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 6:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65572/
> ---
> 
> (Updated Feb. 8, 2018, 6:30 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8552
> https://issues.apache.org/jira/browse/MESOS-8552
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two tests are affected by the lingering executor fix
> (#65109). We should wait to make sure tasks are actually launched
> on the executor before trigger the agent failover. Otherwise, the
> container will be shutdown upon re-registration because the executor
> has never received any task.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 
> 
> 
> Diff: https://reviews.apache.org/r/65572/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 65573: Fixed an error in the 1.3.2 CHANGELOG.

[Chun-Hung Hsiao]

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

Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.


Repository: mesos


Description
---

Fixed an error in the 1.3.2 CHANGELOG.


Diffs
-

  CHANGELOG b7c38f54d9f728d13c319daa4e43c658118ff1b7 


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


Testing
---

N/A


Thanks,

Chun-Hung Hsiao

Review Request 65572: Fixed two slave recovery tests.

[Meng Zhu]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

These two tests are affected by the lingering executor fix
(#65109). We should wait to make sure tasks are actually launched
on the executor before trigger the agent failover. Otherwise, the
container will be shutdown upon re-registration because the executor
has never received any task.


Diffs
-

  src/tests/slave_recovery_tests.cpp 77aa60c953bd0769eaba05f001755e4cec9ba028 


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


Testing
---

make check


Thanks,

Meng Zhu

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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65571 was successfully built and tested.

Reviews applied: `['65571']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 4:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65571/
> ---
> 
> (Updated Feb. 8, 2018, 4:50 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/1/
> 
> 
> Testing
> ---
> 
> `./mesos-tests`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

[Akash Gupta]

> On Feb. 7, 2018, 1:35 p.m., Alexander Rukletsov wrote:
> > src/checks/checker.cpp
> > Line 86 (original), 89 (patched)
> > 
> >
> > please no abbreviations like this, it is unclear what this function 
> > does. Is it a factory? Shall it be better be lambda rather than a free 
> > function?
> > 
> > Also, I see a similar function in `HealthCheck`. Is there any way you 
> > can move duplicate code into the lower level `CheckerProcess`?
> 
> Akash Gupta wrote:
> Yeah, it's a factory function. I think both of your issues can be 
> resolved by passing the `CheckInfo` struct into the `CheckerProcess` class 
> and having the `HealthChecker` use the old `toCheckInfo` function to convert 
> the `HealthCheckInfo` to `CheckInfo`. That should avoid the code duplication 
> too.

Oh it looks like `CheckInfo` doesn't have the http scheme or the ipv6 field. 
Since there's TODOs for gkleiman and qianzhang changing the CheckInfo API to 
include those, I think it's fair for the `CheckerProcess` constructor to just 
include them for now.


- Akash


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


On Feb. 8, 2018, 5:49 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> ---
> 
> (Updated Feb. 8, 2018, 5:49 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
> ---
> 
> 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 65395: Refactored health checks to cleanly separate each different check.

[Akash Gupta]

> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.hpp
> > Lines 143-148 (original)
> > 
> >
> > Leaving a note to make sure this comment was kept around.

thanks for catching this.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 253 (patched)
> > 
> >
> > s/E,F/E, F/g (See, I read the whole comment block!)
> > 
> > Hm, I am left wondering what the `/-` means for the `Nested` row. I 
> > assume that it means "not a thing on Windows", but we should probably 
> > exlicitly state it.

yeah it means "not implemented", but I'll add a note.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 258-271 (patched)
> > 
> >
> > I'm not generally a fan of capture-automatically semantics. I'm left 
> > wondering what exactly we're capturing by copy here, if anything (`this` 
> > maybe?).

You need `this` and the check (`check::Command& cmd` in this case). For the 
capture semantics, I'm interpretting this statement from the mesos style guide, 
"Prefer default capture by value, explicit capture by value, then capture by 
reference. To avoid dangling-pointer bugs, never use default capture by 
reference," as to acutally prefer capture-automatically semantics for value 
types.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 415 (patched)
> > 
> >
> > Is this even reachable?

Yeah the above return is `#ifdef __linux__`, so this part is hit by non Linux 
platforms. Logically, it won't probably get hit since Windows doesn't have 
namespaces, but the compiler doesn't know that.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 435 (patched)
> > 
> >
> > Ditto above (sorry). Though that type is awful enough, perhaps we want 
> > to make an exception.

Yeah, it's pretty ugly, but I guess I'll still add the type back to the left.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 807 (patched)
> > 
> >
> > IIRC this is a struct, should this be a const-ref?

We discussed this and it's best to keep passing these by value to avoid 
dangling refs.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/health_checker.cpp
> > Lines 187-214 (patched)
> > 
> >
> > Where else did I read this code...

Yup, I'm gonna move it inside the CheckerProcess to avoid code duplication.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/docker/executor.cpp
> > Lines 658-664 (patched)
> > 
> >
> > Nit: Style-wise, can we brace-construct this whole struct? (Should we? 
> > Could it become const-qualified if we did?)

yeah it can be const.


- Akash


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


On Feb. 8, 2018, 5:49 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> ---
> 
> (Updated Feb. 8, 2018, 5:49 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
> ---
> 
> 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 

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

[Akash Gupta]

> On Feb. 2, 2018, 9:53 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.hpp
> > Lines 47 (patched)
> > 
> >
> > Should we tag this so a push to their image doesn't break us?

I'm not sure how to solve this problem. The newer images are not backwards 
compatible, so fixed tags will always break on newer Windows releases. 
Personally, I think it's safest to use this tag. Since we require the 
semi-annual channel anyway for this feature, I think the windows hosts will be 
updated and then we'll just pick up the latest powershell image. I don't think 
we can have a concrete solution until the Windows containers guy figure out 
what they want to do with container compatibility with different Windows host.


> On Feb. 2, 2018, 9:53 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 1144-1151 (patched)
> > 
> >
> > I still feel like there's a better way to output this status code.
> > 
> > 
> > From command prompt:
> > ```
> > C:\Users\andrew>pwsh -c (Invoke-WebRequest -Uri google.com 
> > -UseBasicParsing -SkipCertificateCheck).StatusCode
> > 200
> > 
> > C:\Users\andrew>
> > ```
> > 
> > What's the problem with the output number?

It's "200" in UTF-16, while the common code parsing the http status is 
expecting it in UTF-8, why is why there's all that powershell stuff to write to 
file as ascii. If there is a powershell string.utf16_to_utf8(), I can use that.


- Akash


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


On Feb. 8, 2018, 5:51 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 8, 2018, 5:51 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/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

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. 8, 2018, 5:51 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

rebased and responded to 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/7/

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


Testing
---


Thanks,

Akash Gupta

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

[Akash Gupta]

> On Feb. 2, 2018, 9:43 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Line 527 (original), 528-535 (patched)
> > 
> >
> > I'm not quite following this. Why do we always push `command.value()` 
> > then?

I think the original implementation was incorrectly unconditionally pushing 
`command.value()`. The `docker->run` logic was only pushing `command.value()` 
if it existed, so I made this match that logic for consistency. The 
documentation for the `CommandInfo` protobuf just says that the `.value()` and 
`.arguments()` are passed into `execlp` as `execlp(commandInfo.value(), 
commandInfo.arguments...)`, which doesn't make any sense for docker. The 
comment just documents how the protobuf is interpretted inside the executor. 
Also, I don't know if this segment actually gets hit since marathon states that 
all the health checks use the shell form instead of the exec form.


> On Feb. 2, 2018, 9:43 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 535-537 (original), 543-547 (patched)
> > 
> >
> > Can you explain why this was changed a little more? Maybe an example 
> > would help.

Originally, it was setting the `docker exec` in `shell` mode, which wrapped it 
with `sh -c ''`. So, the command was `sh -c 'docker exec sh -c "  "'`, 
which was breaking on Windows due to quoting. Now, it skips the `sh` wrapper 
and calls `docker exec sh -c ''` (in exec form, so it's actually 
`["docker", "exec", "sh", "-c", cmd]`) directly.

This fixes a couple of things:
 - The tests will actually pass on Windows now. Before, due to the crazy 
quoting, the tests were failing.
 - On Linux, your command can now actually have `"` in it instead of having to 
escape it. For example, `echo "Hello World!"` was originally getting changed to 
`sh -c 'docker exec sh -c " echo "Hello World!" "'`, which is broken.
 - On Linux, the mesos-agent shell doesn't accidently interpret the command 
anymore. For example, if your command was `ls $HOME`, originally, it was 
running `sh -c 'docker exec sh -c " ls $HOME "'`, which meant that `$HOME` was 
being   evaluated in the context of the mesos-agent due to the first `sh 
-c` instead of the docker container.

I've seen a couple of issues caused due to the weird quoting. ( 
https://issues.apache.org/jira/browse/MESOS-4812 and 
https://github.com/mesosphere/marathon/issues/5136)


- Akash


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


On Feb. 8, 2018, 5:50 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> ---
> 
> (Updated Feb. 8, 2018, 5:50 p.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/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

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

[Akash Gupta]

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

(Updated Feb. 8, 2018, 5: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
---

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 (updated)
-

  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


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

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


Testing
---

make check


Thanks,

Akash Gupta

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

[Akash Gupta]

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

(Updated Feb. 8, 2018, 5:50 p.m.)


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


Changes
---

responded to feedback


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 (updated)
-

  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


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

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


Testing
---

make check


Thanks,

Akash Gupta

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

[Akash Gupta]

> On Feb. 2, 2018, 9:38 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 523-525 (patched)
> > 
> >
> > It's kind of funny they're quoting this. Not something to fix here, but 
> > it's weird to have hard-coded `sh -c`.
> > 
> > Joe and I have talked about making the shell switchable (so you can 
> > switch between cmd.exe and PowerShell on Windows, or sh and bash etc. on 
> > Linux). Maybe we should file an issue so can start tracking hard-coded 
> > shells with a `TODO`?

Yeah that sounds good to me. It's going to call os::Shell in the next patch, so 
the hard-coded `sh -c` is going to go away.


> On Feb. 2, 2018, 9:38 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 529 (patched)
> > 
> >
> > Is `command.arguments()` iterable? If so, we could use the vector's 
> > reange constructor.

we can do `commandArguments.insert(commandArguments.end(), 
command.arguments().begin(), command.arguments().end());`. But since it's going 
to changed anyway in the next patch, I'll do it there.


- Akash


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


On Feb. 8, 2018, 5:50 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65396/
> ---
> 
> (Updated Feb. 8, 2018, 5: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
> ---
> 
> 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

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

[Akash Gupta]

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

(Updated Feb. 8, 2018, 5:49 p.m.)


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


Changes
---

rebased and responded feedback


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 (updated)
-

  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/

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


Testing
---

make check


Thanks,

Akash Gupta

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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65570 was successfully built and tested.

Reviews applied: `['65570']`

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

- Mesos Reviewbot Windows


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

Re: Review Request 65569: Install ping for docker build.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65569 was successfully built and tested.

Reviews applied: `['65569']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 3:39 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65569/
> ---
> 
> (Updated Feb. 8, 2018, 3:39 p.m.)
> 
> 
> Review request for mesos, Michael Park and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By default Ubuntu docker image does not contain `ping`.
> We need to manually install it with `iputils-ping`.
> 
> Refs: https://stackoverflow.com/a/39901446/1387612
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh 126d4e781086636a318afc9199a3566bdb578051 
> 
> 
> Diff: https://reviews.apache.org/r/65569/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

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

[Mesos Reviewbot]

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



Patch looks great!

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

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. 8, 2018, 5:55 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65556/
> ---
> 
> (Updated Feb. 8, 2018, 5:55 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
> ---
> 
> 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/1/
> 
> 
> Testing
> ---
> 
> `sudo make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65570: WIP: 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. 8, 2018, 11:41 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

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


Diffs
-

  src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 


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


Testing
---


Thanks,

Qian Zhang

Review Request 65570: WIP: 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/
---

Review request for mesos.


Repository: mesos


Description
---

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


Diffs
-

  src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 


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


Testing
---


Thanks,

Qian Zhang

Review Request 65569: Install ping for docker build.

[Tomasz Janiszewski]

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

Review request for mesos, Michael Park and Vinod Kone.


Repository: mesos


Description
---

By default Ubuntu docker image does not contain `ping`.
We need to manually install it with `iputils-ping`.

Refs: https://stackoverflow.com/a/39901446/1387612


Diffs
-

  support/docker-build.sh 126d4e781086636a318afc9199a3566bdb578051 


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


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 65314: Removed code which is not used.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65314 was successfully built and tested.

Reviews applied: `['65310', '65311', '65312', '65313', '65314']`

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

- Mesos Reviewbot Windows


On Feb. 8, 2018, 12:59 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65314/
> ---
> 
> (Updated Feb. 8, 2018, 12:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The introduction of the `ObjectApprovers` abstraction rendered the
> `AuthorizationAcceptor` class oboslete. After the refactor of the code
> the acceptor class was no longer used, nor were the helper functions
> built around it.
> 
> This patch removes that obsolete code.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
>   src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 
> 
> 
> Diff: https://reviews.apache.org/r/65314/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65314: Removed code which is not used.

[Alexander Rojas]

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

(Updated Feb. 8, 2018, 1:59 p.m.)


Review request for mesos, Benjamin Hindman and Greg Mann.


Changes
---

Forcing reviewboard to run.


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


Repository: mesos


Description
---

The introduction of the `ObjectApprovers` abstraction rendered the
`AuthorizationAcceptor` class oboslete. After the refactor of the code
the acceptor class was no longer used, nor were the helper functions
built around it.

This patch removes that obsolete code.


Diffs (updated)
-

  src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
  src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 65313: Refactored authorization logic in the agent.

[Alexander Rojas]

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

(Updated Feb. 8, 2018, 1:59 p.m.)


Review request for mesos, Benjamin Hindman and Greg Mann.


Changes
---

Fixed failed `DefaultExecutor` tests.


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


Repository: mesos


Description
---

This patch makes uses of the new `ObjectApprovers` class which greatly
simplifies the logic for constructing and using authorization.


Diffs (updated)
-

  src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
  src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
  src/slave/slave.cpp f98f37321872d090176b7cc50873fc3c627773f5 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 62800: Created cgroups under systemd hierarchy in LinuxLauncher.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62798, 62799, 62800]

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. 7, 2018, 8:01 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62800/
> ---
> 
> (Updated Feb. 7, 2018, 8:01 p.m.)
> 
> 
> Review request for mesos, James Peach, Joris Van Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-7990
> https://issues.apache.org/jira/browse/MESOS-7990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added the support for systemd hierarchy in LinuxLauncher.
> It created the same cgroup layout under the systemd hierarchy (if
> systemd is enabled) as that in the freezer hierarchy.
> 
> This can give us a bunch of benefits:
> 1) systemd-cgls can list mesos container processes.
> 2) systemd-cgtop can show stats for mesos containers.
> 3) Avoid the pid migration issue described in MESOS-3352.
> 
> For example:
> 
> ```
> [jie@core-dev ~]$ systemd-cgls
> |-1 /usr/lib/systemd/systemd --system --deserialize 20
> |-mesos
> |  |-8282b91a-5724-4964-a623-7c6bd68ff4ad
> |  |-31737 /usr/libexec/mesos/mesos-containerizer launch
> |  |-31739 mesos-default-executor --launcher_dir=/usr/libexec/mesos
> |  |-mesos
> | |-8555f4af-fa4f-4c9c-aeb3-0c9f72e6a2de
> |   |-31791 /usr/libexec/mesos/mesos-containerizer launch
> |   |-31793 sleep 1000
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e319894874e628beda6dc305462af0c274fd7b 
> 
> 
> Diff: https://reviews.apache.org/r/62800/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> I also use the steps describe in MESOS-3352 to repo the systemd pid migration 
> issue. Didn't observe it after applying the patch.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>