Re: Review Request 58889: Remove FlagsFileTest.JSONFile from Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58673, 58889]

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

- Mesos Reviewbot


On May 2, 2017, 7 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58889/
> ---
> 
> (Updated May 2, 2017, 7 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-5937
> https://issues.apache.org/jira/browse/MESOS-5937
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code was deprecated for Linux, and thus makes no sense
> to port to Windows. This patch removes test from the Windows
> platform completely.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/parse.hpp 
> 65edd86372596c2107e9f29cf27301e025e6620e 
>   3rdparty/stout/tests/flags_tests.cpp 
> e2681f8f68f6478d4c8a20c1e75ddb050d16b56d 
> 
> 
> Diff: https://reviews.apache.org/r/58889/diff/2/
> 
> 
> Testing
> ---
> 
> For Linux, all tests pass. In particular, with FlagsFileTest:
> 
> [--] 2 tests from FlagsFileTest
> [ RUN  ] FlagsFileTest.JSONFile
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W0501 12:55:43.343416 38425 parse.hpp:101] Specifying an absolute filename to 
> read a command line option out of without using 'file:// is deprecated and 
> will be removed in a future release. Simply adding 'file://' to the beginning 
> of the path should eliminate this warning.
> [   OK ] FlagsFileTest.JSONFile (0 ms)
> [ RUN  ] FlagsFileTest.FilePrefix
> [   OK ] FlagsFileTest.FilePrefix (0 ms)
> [--] 2 tests from FlagsFileTest (0 ms total)
> 
> For Windows, where FlagsFileTest.JSONFile is testing deprecated behavior, 
> it's only appropriate for Linux. Thus, it's not run.
> 
> PS C:\mesos\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe 
> --gtest_filter="FlagsFile*"
> Note: Google Test filter = FlagsFile*-
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from FlagsFileTest
> [ RUN  ] FlagsFileTest.FilePrefix
> [   OK ] FlagsFileTest.FilePrefix (3 ms)
> [--] 1 test from FlagsFileTest (3 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (7 ms total)
> [  PASSED  ] 1 test.
> PS C:\mesos\mesos>
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[haosdent huang]

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




CHANGELOG
Lines 186 (patched)


Should we add `MESOS-7453` at here?


- haosdent huang


On May 2, 2017, 11:48 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 2, 2017, 11:48 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[haosdent huang]

> On May 1, 2017, 10:49 p.m., Benjamin Mahler wrote:
> > I'm a bit lost, is there a visual change here? If so, can you include the 
> > screenshots?
> > 
> > Could you spell out the problem? Is it that when trying to copy a path in 
> > the file browser, it includes unwanted spaces?

Oh, yes, it is confuse without any screenshot. I update a screenshot at 
https://reviews.apache.org/r/58874/file/1428/


- haosdent


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


On May 3, 2017, 4:29 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58874/
> ---
> 
> (Updated May 3, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stripped spaces between directory elements in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 
> b9849197227b06df348789a49348e2b5d4cfd2ae 
>   src/webui/master/static/css/mesos.css 
> 9f3de5427071fc61d3791c4bc2a660368c2cd3c2 
> 
> 
> Diff: https://reviews.apache.org/r/58874/diff/2/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> strip_space.gif
>   
> https://reviews.apache.org/media/uploaded/files/2017/05/03/79afd0b1-5fb1-437c-a91c-732009af8fe3__strip_space.gif
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[haosdent huang]

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

(Updated May 3, 2017, 4:29 a.m.)


Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.


Repository: mesos


Description
---

Stripped spaces between directory elements in WebUI.


Diffs
-

  src/webui/master/static/browse.html b9849197227b06df348789a49348e2b5d4cfd2ae 
  src/webui/master/static/css/mesos.css 
9f3de5427071fc61d3791c4bc2a660368c2cd3c2 


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


Testing
---


File Attachments (updated)


strip_space.gif
  
https://reviews.apache.org/media/uploaded/files/2017/05/03/79afd0b1-5fb1-437c-a91c-732009af8fe3__strip_space.gif


Thanks,

haosdent huang

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[haosdent huang]

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

(Updated May 3, 2017, 4:25 a.m.)


Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.


Repository: mesos


Description
---

Stripped spaces between directory elements in WebUI.


Diffs (updated)
-

  src/webui/master/static/browse.html b9849197227b06df348789a49348e2b5d4cfd2ae 
  src/webui/master/static/css/mesos.css 
9f3de5427071fc61d3791c4bc2a660368c2cd3c2 


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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[haosdent huang]

> On May 1, 2017, 8:14 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/browse.html
> > Line 17 (original), 17 (patched)
> > 
> >
> > Could you keep original formatting?

Change to

```
  {{dir}}/
```

Is it match you expection here?


> On May 1, 2017, 8:14 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/css/mesos.css
> > Lines 180 (patched)
> > 
> >
> > I think this should be replaced with `visibility: hidden;`. With 
> > `font-size: 0;` there is different (smaller) padding then before the change.

Oh, I didn't use it because with `visibility: hidden`, the copy content would 
not include `` in my browser. Is it work in your side?


- haosdent


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


On April 30, 2017, 4:29 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58874/
> ---
> 
> (Updated April 30, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stripped spaces between directory elements in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 
> b9849197227b06df348789a49348e2b5d4cfd2ae 
>   src/webui/master/static/css/mesos.css 
> 9f3de5427071fc61d3791c4bc2a660368c2cd3c2 
> 
> 
> Diff: https://reviews.apache.org/r/58874/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58872: Ensured sandbox URI request reroute after fetched `$scope.state`.

[haosdent huang]

> On May 1, 2017, 10:45 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 906 (patched)
> > 
> >
> > Should this be called "update"? It looks to me like it's responsible 
> > for redirecting.
> > 
> > Also, like Tomasz pointed out, I can't really tell what the if 
> > condition below does to fix the problem. Would you mind spelling out the 
> > problem and the fix in the description?

Fixed, thank you.


- haosdent


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


On May 3, 2017, 4:15 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> ---
> 
> (Updated May 3, 2017, 4:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> a021962573d452de1581e6a7717016eac7d0cd85 
> 
> 
> Diff: https://reviews.apache.org/r/58872/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58872: Ensured sandbox URI request reroute after fetched `$scope.state`.

[haosdent huang]

> On May 1, 2017, 8:45 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1019 (patched)
> > 
> >
> > Please add a comment for this condition with information why it can 
> > happen. The comment could be the same as previously used:
> > 
> > > When navigating directly to this page, e.g. pasting the URL into the 
> > browser

Thank you very much, have updated.


> On May 1, 2017, 8:45 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1024 (patched)
> > 
> >
> > It's not obious why we remove update listener on route change start. 
> > Can you add a comment with clarification?

It is because we no need to call this again if route change, so we remove it 
here. We do it in almost places which depends on `$scope.state`.

```
var removeListener = $scope.$on('state_updated', update);
$scope.$on('$routeChangeStart', removeListener);
```

Do you think we need to add comment about this here?


- haosdent


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


On May 3, 2017, 4:15 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> ---
> 
> (Updated May 3, 2017, 4:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> a021962573d452de1581e6a7717016eac7d0cd85 
> 
> 
> Diff: https://reviews.apache.org/r/58872/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58872: Ensured sandbox URI request reroute after fetched `$scope.state`.

[haosdent huang]

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

(Updated May 3, 2017, 4:15 a.m.)


Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.


Changes
---

Address @bmahler, @janisz comments.


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


Repository: mesos


Description (updated)
---

Open sandbox link in a new tab would fail since it depends on agents
information loading finish. This patch fixes it by ensuring the agents
information loaded first and then rerouting the sandbox request.


Diffs (updated)
-

  src/webui/master/static/js/controllers.js 
a021962573d452de1581e6a7717016eac7d0cd85 


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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 58944: Fixed compilation error on recent glibc for device whitelist.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On May 3, 2017, 3:58 a.m., Zhongbo Tian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58944/
> ---
> 
> (Updated May 3, 2017, 3:58 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed compilation error on recent glibc for device whitelist.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 3055b7d41d31a353d672c8934b8545c01b70d2de 
> 
> 
> Diff: https://reviews.apache.org/r/58944/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhongbo Tian
> 
>

Review Request 58944: Fixed compilation error on recent glibc for device whitelist.

[Zhongbo Tian]

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

Review request for mesos.


Repository: mesos


Description
---

Fixed compilation error on recent glibc for device whitelist.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
3055b7d41d31a353d672c8934b8545c01b70d2de 


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


Testing
---


Thanks,

Zhongbo Tian

Re: Review Request 58925: Updated runtime isolators to use new task_environment member.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58923, 58924, 58925]

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

- Mesos Reviewbot


On May 2, 2017, 5:28 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58925/
> ---
> 
> (Updated May 2, 2017, 5:28 p.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
> ffaec5a0112db36c1e9123e361ada63a4aedf0ad 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 08350e638a0f20746e369cdc78c96126f2e1df3f 
> 
> 
> Diff: https://reviews.apache.org/r/58925/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 58916: Fixed a minor bug in setting the agent's `totalResources`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58916]

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

- Mesos Reviewbot


On May 2, 2017, 8:26 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58916/
> ---
> 
> (Updated May 2, 2017, 8:26 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the following branch is taken, `totalResources` would be set
> to `resources.get()` which is not equal to `info.resources()`.
> 
> ```cpp
>   if (HookManager::hooksAvailable()) {
> info.mutable_resources()->CopyFrom(
> HookManager::slaveResourcesDecorator(info));
>   }
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
> 
> 
> Diff: https://reviews.apache.org/r/58916/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58880: Added Secret to Image::Docker in v1/mesos.proto.

[Vinod Kone]

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



Make sure to update unversioned mesos.proto as well.


include/mesos/v1/mesos.proto
Lines 2134 (patched)


s/is never used/has never been used/



include/mesos/v1/mesos.proto
Lines 2135 (patched)


s/the `Secret::Value` field//



include/mesos/v1/mesos.proto
Lines 2140-2143 (patched)


I would rephrase this as follows

// Docker config containing credentails to authenticate with docker 
registry.
//
// The secret is expected to be in docker config file in JSON format with 
UTF-8 character encoding.


- Vinod Kone


On April 30, 2017, 11:23 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58880/
> ---
> 
> (Updated April 30, 2017, 11:23 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, Kapil Arya, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a followup patch for https://reviews.apache.org/r/58775/.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto f7c05a82f8265aedc0bd8fd20dd30e21af46e775 
> 
> 
> Diff: https://reviews.apache.org/r/58880/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 56895: Allow agents to recover slave state post a reboot.

[Vinod Kone]

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




src/slave/slave.hpp
Lines 333 (patched)


Can you add a comment on what this represents?



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


s/info/info after a reboot/

also please log the `future.failure()` here? are we sure that a failed 
future here can only happen due to incompatibile agent info?



src/slave/slave.cpp
Line 5954 (original), 5963 (patched)


As discussed offline, we should continue registering as new agent instead 
of exiting here.



src/slave/state.hpp
Lines 310 (patched)


s/hasRebooted/rebooted/


- Vinod Kone


On April 26, 2017, 6:16 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated April 26, 2017, 6:16 p.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With partition awareness, the agents are now allowed to re-register
> after they have been marked Unreachable. The executors are anyway
> terminated on the agent when it reboots so there is no harm in
> letting the agent keep its SlaveID, re-register with the master
> and reconcile the lost executors. This is a pre-requisite for
> supporting persistent/restartable tasks in mesos.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 4ff522e75bc8de34fe2e7720bdd8ce3d32cbf803 
>   src/slave/state.hpp a497ce1f58fb8dc7718ee5bb10bc62dd7479efa5 
>   src/slave/state.cpp 33dcc7a148f9a6b1a3216cce45710da8fd819ba6 
>   src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
>   src/tests/slave_recovery_tests.cpp 53f33a2b0411c8158326074ce043c7b1dbeef5b4 
> 
> 
> Diff: https://reviews.apache.org/r/56895/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 58880: Added Secret to Image::Docker in v1/mesos.proto.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On April 30, 2017, 11:23 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58880/
> ---
> 
> (Updated April 30, 2017, 11:23 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, Kapil Arya, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a followup patch for https://reviews.apache.org/r/58775/.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto f7c05a82f8265aedc0bd8fd20dd30e21af46e775 
> 
> 
> Diff: https://reviews.apache.org/r/58880/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 58939: Filesystem isolation check for Mesos containerizer under Linux.

[Chun-Hung Hsiao]

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

Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Bugs: mesos-7374
https://issues.apache.org/jira/browse/mesos-7374


Repository: mesos


Description
---

Checked if the 'filesystem/linux' isolator is active when launching a
mesos containerizer with an image under Linux. This prevents the
executor from messing up with the host filesystem when launching nested
containers with failures.

The check is placed in `MesosContainerizerProcess::prepare()`, which is
executed after provisioning but before launching, since the isolator is
not required for the provisioner, so the existing unit tests for
provisioners won't fail.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
be45fc59027f176b43b767e9441fd8089ceec7b4 


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


Testing
---

sudo make check
Manual tested a simplified case of mesos-7374.


Thanks,

Chun-Hung Hsiao

Re: Review Request 58928: Update process tests to use a non-zero UPID.

[James Peach]

> On May 2, 2017, 9:35 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/tests/test_linkee.cpp
> > Lines 131-134 (patched)
> > 
> >
> > Is this useful? It seems like we would need to pick the "right one" for 
> > whether we're going to talk locally or remotely? Or?

This code is lifted verbatim from `process::initialize()`.


- James


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


On May 2, 2017, 6 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58928/
> ---
> 
> (Updated May 2, 2017, 6 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the remote process tests were using a UPID with a zero
> IP address field. In order to enable subsequent IP address
> validation in the libprocess receiver, update these tests to
> use a UPID containing the real IP address of the sender.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 921d67695bc0e4d601e9f74fbc625d69bf36ba50 
> 
> 
> Diff: https://reviews.apache.org/r/58928/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Michael Park]

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

(Updated May 2, 2017, 4:48 p.m.)


Review request for mesos and Neil Conway.


Changes
---

Dotted all the things.


Repository: mesos


Description (updated)
---

CHANGELOG for 1.3.0 release.


Diffs (updated)
-

  CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 


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

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


Testing
---


Thanks,

Michael Park

Review Request 58942: CHANGELOG for 1.3.0 release.

[Michael Park]

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

Review request for mesos and Neil Conway.


Repository: mesos


Description
---

See summary.


Diffs
-

  CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 


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


Testing
---


Thanks,

Michael Park

Review Request 58941: Exposed full unreserved resources in /state endpoint on agent.

[Vinod Kone]

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

Review request for mesos, Jie Yu and Neil Conway.


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


Repository: mesos


Description
---

The JSON key for this information is "unreserved_resources_full".


Diffs
-

  src/slave/http.cpp 2ce345d9958a867ece835e47287215cefd561abf 
  src/tests/reservation_endpoints_tests.cpp 
cc8499a5ec05cf7b2283c075e47298918f50bd24 


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


Testing
---

make check


Thanks,

Vinod Kone

Review Request 58940: Exposed full unreserved resources in /slaves endpoint on master.

[Vinod Kone]

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

Review request for mesos, Jie Yu and Neil Conway.


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


Repository: mesos


Description
---

The JSON key for this information is "unreserved_resources_full".


Diffs
-

  src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
  src/tests/persistent_volume_endpoints_tests.cpp 
1237d081d781948975f66a8240ae9bdb5e819a3b 


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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 58137: CLI: Added 'mesos config show' command to display the config file.

[Kevin Klues]

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




src/cli_new/lib/cli/plugins/config/main.py
Lines 87 (patched)


We shoud use the syntax where we name the placeholder instead of leaving it 
anonymous, i.e.:
```
sys.stdout.write("{stream}\n".format(stream=stream.read().rstrip()))
```


- Kevin Klues


On April 12, 2017, 9:21 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58137/
> ---
> 
> (Updated April 12, 2017, 9:21 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command displays the contents of the user-defined config.toml file.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/plugins/config/main.py 
> d95a36f4a66c66b4477c6816b7fa5a721f9212f7 
> 
> 
> Diff: https://reviews.apache.org/r/58137/diff/6/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/tests/base.py
Lines 116-117 (patched)


We should not end strings with a space (i.e. "something "). The space 
should always begin on the next line. Also, we should try and balance the 
string across the two lines, i.e.:

```
if Master.count > 0:
raise CLIException("Creating more than one master"
   " is currently not possible")
```



src/cli_new/lib/cli/tests/base.py
Lines 165-168 (patched)


Let's pull these constants out into module variables.

```
DEFAULT_MASTER_IP = 127.0.0.1
DEFAULT_MASTER_PORT = 9090

DEFAULT_AGENT_IP = 127.0.0.1
DEFAULT_AGENT_PORT = 9091
```



src/cli_new/lib/cli/tests/base.py
Lines 177 (patched)


Here too.



src/cli_new/lib/cli/tests/base.py
Lines 257 (patched)


here too



src/cli_new/lib/cli/tests/base.py
Lines 386 (patched)


Let's use the `os.path.join` here.


- Kevin Klues


On April 25, 2017, 10:18 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated April 25, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/1/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58394: Setup new directory for python http client lib in src/python.

[Kevin Klues]

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




src/python/.gitignore
Lines 1 (patched)


Why do you need .virtualenv in this folder? I wouldn't expect us to create 
a .virtualenv directory anywhere within this subolder (at least not yet). Once 
we move the cli code under this folder it might make sense. But not yet.



src/python/lib/mesos/__init__.py
Lines 1 (patched)


I would exclude this from this commit.
It is not used anywhere, therefore it should not be included until the 
commit where it is used.


- Kevin Klues


On April 12, 2017, 9:44 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58394/
> ---
> 
> (Updated April 12, 2017, 9:44 p.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7310
> https://issues.apache.org/jira/browse/MESOS-7310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Setup new directory for python http client lib in src/python.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
>   src/python/.gitignore PRE-CREATION 
>   src/python/lib/mesos/__init__.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58394/diff/1/
> 
> 
> Testing
> ---
> 
> Under src/cli_new:
> 1\. ./bootstrap
> 2\. . ./activate
> 3\. python
> 4\. >>> import mesos
> 5\. >>> mesos.\_\_path\_\_
> 6\. verify that the path printed out is indeed at src/python/lib/mesos
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/tests/base.py
Lines 133 (patched)


We should use the `.format` syntax here. Also, we should use `os.path.join`.


- Kevin Klues


On April 25, 2017, 10:18 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated April 25, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/1/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58719: CLI: Added utility functions related to HTTP.

[Kevin Klues]

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


Ship it!




I think some of this will be superceded by the work that Eric Chung is doing to 
provide a more sophisticated HTTP library for the CLI. But this should work for 
now until that work lands.

- Kevin Klues


On April 25, 2017, 10:18 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58719/
> ---
> 
> (Updated April 25, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used by future plugins and tests.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/http.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58719/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58137: CLI: Added 'mesos config show' command to display the config file.

[Kevin Klues]

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




src/cli_new/lib/cli/plugins/config/main.py
Lines 83-84 (patched)


You need to check that this file exists and raise an appropriate 
CLIException if it does not.


- Kevin Klues


On April 12, 2017, 9:21 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58137/
> ---
> 
> (Updated April 12, 2017, 9:21 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command displays the contents of the user-defined config.toml file.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/plugins/config/main.py 
> d95a36f4a66c66b4477c6816b7fa5a721f9212f7 
> 
> 
> Diff: https://reviews.apache.org/r/58137/diff/6/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 57952: Added 'config' plugin to the new CLI.

[Kevin Klues]

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



As part of this patch , I expected `main.py` to be updated to explicitly load 
only the config plugin and then have the config plugin load the real set of 
plugins and pass them back to main. I.e. per our discussion on slack:

```
Maybe the right answer is to:
1) Move all TOML file parsing / validation out of settings.py and into the 
config plugin
2) Have main() explicitly instantiate the Config plugin by name
3) Replace the following call in `main()` from:
# Initialize the various plugins.
plugins = mesos.util.import_modules(settings.PLUGINS, "plugins")

to
plugins = ConfigPlugin(settings).plugins([])

3) The `ConfigPlugin.plugins()` command will then be in charge running 
`mesos.util.import_modules(plugins, "plugins")` over all plugins (both builtin 
and those found in the TOML file) to validate that they are importable and then 
return their loaded modules.
4) The main function would then process as before from there (edited)

[16:41] 
When `mesos config plugins` is invoked by the user, it can print out, e.g.:
NAMEDESCRIPTION
agent   Agent specific commands for the Mesos CLI
cluster Cluster specific commands for the Mesos CLI
container   Container specific commands for the Mesos CLI

which it can parse from the loaded modules from `PLUGIN_NAME` and `SHORT_HELP` 
(edited)

[16:44] 
or if there is an error with loading one of the plugins it can raise a 
CLIExpection() with a meaningful error message
```

- Kevin Klues


On April 12, 2017, 9:19 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57952/
> ---
> 
> (Updated April 12, 2017, 9:19 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Bugs: MESOS-7269
> https://issues.apache.org/jira/browse/MESOS-7269
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used to show the plugins available for the user.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/settings.py 2f6162edc1722054bc44ad25956e6fe666d36c7f 
>   src/cli_new/lib/cli/plugins/config/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/plugins/config/main.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57952/diff/6/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58818: Ensured DEBUG container shares MESOS_SANDBOX with its parent.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58818/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7355
> https://issues.apache.org/jira/browse/MESOS-7355
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58818/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS and Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58892: Added C++11 scoped enumeration to style guide.

[Jiang Yan Xu]

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

(Updated May 2, 2017, 3:11 p.m.)


Review request for mesos and Michael Park.


Changes
---

Comments.


Repository: mesos


Description
---

We've been using it for new code now.


Diffs (updated)
-

  docs/c++-style-guide.md ccb81f48e2edc9c1e7c328cc26e44d658b4c43b4 


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

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


Testing
---

Markdown viewer.


Thanks,

Jiang Yan Xu

Re: Review Request 58892: Added C++11 scoped enumeration to style guide.

[Jiang Yan Xu]

> On May 2, 2017, 12:24 a.m., Michael Park wrote:
> > docs/c++-style-guide.md
> > Lines 649 (patched)
> > 
> >
> > Maybe just mention `enum class` to eliminate confusion as to which one 
> > should be used?

+1.


- Jiang Yan


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


On May 1, 2017, 2:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58892/
> ---
> 
> (Updated May 1, 2017, 2:07 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We've been using it for new code now.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md ccb81f48e2edc9c1e7c328cc26e44d658b4c43b4 
> 
> 
> Diff: https://reviews.apache.org/r/58892/diff/1/
> 
> 
> Testing
> ---
> 
> Markdown viewer.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 58673: Fix FlagsFileTest to check for absolute path properly on Windows.

[Jeff Coffler]

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

(Updated May 2, 2017, 9:46 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


Changes
---

Resolved change for Joe (deprecated feature on Linux, not Windows).


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


Repository: mesos


Description
---

Added 3rdparty/stout/tests/path_tests.cpp tests to Windows platform,
but disabled tests that did not pass. Enabled absolute path tests,
adding tests that were appropriate for the Windows platform.

Note that, for Windows, absolute paths may not be valid. For example,
a path like "\\?\abc:file.txt" is not valid. In this case, the
path::absolute method is undefined; the expectation is that it is
called with valid paths (absolute or relative, but valid).


Diffs (updated)
-

  3rdparty/stout/include/stout/flags/parse.hpp 
65edd86372596c2107e9f29cf27301e025e6620e 
  3rdparty/stout/include/stout/path.hpp 
2d2088aadfa1ea82c59424242671c4fb655dede1 
  3rdparty/stout/tests/CMakeLists.txt 4bbe713f259e7858d423dcb33956d41e62a915eb 
  3rdparty/stout/tests/flags_tests.cpp e2681f8f68f6478d4c8a20c1e75ddb050d16b56d 
  3rdparty/stout/tests/path_tests.cpp 0490d93908566c46a10d91b05790e5a7f2f289bc 


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

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


Testing
---

Passes `make check` on the Linux platform.

On Windows, the FlagsFileTest.JSONFile now passes, along with new 
PathTest.Absolute tests that I added due to new path::absolute handling on the 
Windows platform.

PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe 
--gtest_filter="*FlagsFileTest*"
Note: Google Test filter = *FlagsFileTest*-
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from FlagsFileTest
[ RUN  ] FlagsFileTest.JSONFile
WARNING: Logging before InitGoogleLogging() is written to STDERR
W0425 10:05:20.959357 1060440 parse.hpp:97] Specifying an absolute filename to 
read a command line option out of without using 'file:// is deprecated and will 
be removed in a future release. Simply adding 'file://' to the beginning of the 
path should eliminate this warning.
[   OK ] FlagsFileTest.JSONFile (9 ms)
[ RUN  ] FlagsFileTest.FilePrefix
[   OK ] FlagsFileTest.FilePrefix (6 ms)
[--] 2 tests from FlagsFileTest (18 ms total)

[--] Global test environment tear-down
[==] 2 tests from 1 test case ran. (23 ms total)
[  PASSED  ] 2 tests.
PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe 
--gtest_filter="*PathTest*"
Note: Google Test filter = *PathTest*-
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from PathTest
[ RUN  ] PathTest.Absolute
[   OK ] PathTest.Absolute (1 ms)
[ RUN  ] PathTest.Comparison
[   OK ] PathTest.Comparison (0 ms)
[--] 2 tests from PathTest (2 ms total)

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

  YOU HAVE 4 DISABLED TESTS

PS C:\mesos>


Thanks,

Jeff Coffler

Re: Review Request 58928: Update process tests to use a non-zero UPID.

[Benjamin Mahler]

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




3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1770 (patched)


Rather than making up a number here (99), it seems we should get this from 
the http::Connection. Have you looked into exposing it?



3rdparty/libprocess/src/tests/test_linkee.cpp
Lines 130 (patched)


Are "missing" and "0.0.0.0" different cases?



3rdparty/libprocess/src/tests/test_linkee.cpp
Lines 131-134 (patched)


Is this useful? It seems like we would need to pick the "right one" for 
whether we're going to talk locally or remotely? Or?



3rdparty/libprocess/src/tests/test_linkee.cpp
Lines 139 (patched)


Failed to gethostname



3rdparty/libprocess/src/tests/test_linkee.cpp
Lines 143-144 (patched)


Do you know why this returns only 1 IP? Isn't it possible for there to be 
more than 1?


- Benjamin Mahler


On May 2, 2017, 6 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58928/
> ---
> 
> (Updated May 2, 2017, 6 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the remote process tests were using a UPID with a zero
> IP address field. In order to enable subsequent IP address
> validation in the libprocess receiver, update these tests to
> use a UPID containing the real IP address of the sender.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 921d67695bc0e4d601e9f74fbc625d69bf36ba50 
> 
> 
> Diff: https://reviews.apache.org/r/58928/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

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

(Updated May 2, 2017, 9:33 p.m.)


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


Changes
---

Rebase on upstream.


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


Repository: mesos


Description
---

- Previously, rmdir operations were serialized
  through the garbage collector. Expensive removal
  events had the potential to delay task launch.
- MESOS-6549


Diffs (updated)
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 


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

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


Testing
---

`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
--gtest_break_on_failure`


Thanks,

Jacob Janco

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/version.hpp
Lines 77 (patched)


See below?



3rdparty/stout/include/stout/version.hpp
Lines 102 (patched)


I guess this breaks for the negative case? E.g. 1.0.0--1.-1

We'll treat the -1's as large numbers based on the numify test case you 
showed earlier? Rather than being invalid (no negative allowed) or non-numeric 
(due to the hyphen). Not clear which one is correct from the spec AFAICT, 
unfortunately :(



3rdparty/stout/include/stout/version.hpp
Lines 104-111 (patched)


Did you consider making this an optional validation of validateIdentifier? 
That would allow us to do invariant CHECKs in the constructor, see my other 
comment.



3rdparty/stout/include/stout/version.hpp
Line 62 (original), 131 (patched)


Do we need a TODO here to reject negatives? Or do you want to just 
implement that now?



3rdparty/stout/include/stout/version.hpp
Line 66 (original), 137-144 (patched)


I guess we could do this in a separate patch, to make it clearer that we're 
making a change to the existing behavior rather than only adding build and 
prerelease label support?



3rdparty/stout/include/stout/version.hpp
Lines 73-76 (original), 156-167 (patched)


It would be nice to catch errors early here and do the CHECKs in the 
constructor in case someone tries to construct a malformed version:

```
{
  foreach (const std::string& identifier, prerelease) {
CHECK_NONE(validateIdentifier(identifier, true)); // 
disallow_leading_zeros=true
  }

  foreach (const std::string& identifier, build) {
CHECK_NONE(validateIdentifier(identifier));
  }
}
```

Separately from your change, it would be nice to also use unsigned integers 
to avoid needing to validate against negative numbers, but we can do this in a 
separate patch.



3rdparty/stout/include/stout/version.hpp
Lines 243-264 (patched)


Should we keep the notion of "other" here rather than "1" and "2"?

I found the nesting here to make this a little hard to follow, maybe we can 
use a flat list of cases to make this easier to read?

```
  if (identifier.isSome() && other_identifier.isSome()) {
// Both are numeric.
if (identifier.get() != other_identifier.get()) {
  return identifier.get() < other_identifier.get();
}
  } else if (identifier.isSome()) {
// If `this` identifier is numeric but `other` is not, `this < 
other`.
return true;
  } else if (other_identifier.isSome()) {
// If `other` identifier is numeric but `this` is not, `other < 
this`.
return false;
  } else {
// Neither identifier is numeric, so compare them via ASCII sort.
if (prerelease.at(i) != other.prerelease.at(i)) {
  return prerelease.at(i) < other.prerelease.at(i);
}
  }
```



3rdparty/stout/tests/version_tests.cpp
Lines 90-91 (original), 149-152 (patched)


Oh.. I see that negatives are being caught correctly, I didn't realize when 
reading the code that we were catching them due to the pre-release label 
parsing. 

Hm.. maybe we need a little note about that in the major,minor,patch 
numification loop?


- Benjamin Mahler


On May 2, 2017, 8 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> ---
> 
> (Updated May 2, 2017, 8 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kapil Arya.
> 
> 
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
> 
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
> 
> 
> Diffs
> -
> 
>   3rdparty/st

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[James Peach]

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

(Updated May 2, 2017, 9:02 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


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

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


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58587: Clarified comments about resource operations through operator API.

[Jiang Yan Xu]

> On May 2, 2017, 12:13 a.m., Michael Park wrote:
> > src/master/http.cpp
> > Lines 4870-4872 (patched)
> > 
> >
> > Thanks for adding this! This is the issue that I was considering filing 
> > a JIRA ticket for.

Do you have some ideas on how to improve this? I guess a JIRA would be good 
even with this NOTE.


> On May 2, 2017, 12:13 a.m., Michael Park wrote:
> > src/master/http.cpp
> > Lines 4884-4889 (patched)
> > 
> >
> > I'm not quite following this example. Specifically, I don't get what "a 
> > persistent volume can be destroyed if it is not allocated" means..

Ok I guess I shoud say "a persistent volume can be destroyed only if it is not 
used". In this case, it's not going to be pending offers so you would 
unncessarily rescind all offers.


- Jiang Yan


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


On May 1, 2017, 2:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58587/
> ---
> 
> (Updated May 1, 2017, 2:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Seems like the comments about "virtually always win the race against
> 'allocate'" could be made more precise in helping people understand
> how it works.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
> 
> 
> Diff: https://reviews.apache.org/r/58587/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Benjamin Mahler]

> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 156-157 (patched)
> > 
> >
> > In general we try to open and close the quote on the same line if 
> > possible, as it tends to reduce the amount of times we forget to close the 
> > quote, ditto elsewhere
> 
> Neil Conway wrote:
> The annoying thing with moving the single quote to the next line is that 
> you can't easily `+` two character literals together, so you end up with
> 
> ```
> return Error("Identifier contains illegal character: " +
>  string("'") + stringify(*firstInvalid) + "'");
> ```
> 
> which IMO is harder to read overall.

Per offline discussion, string literals can be concatenated without the + 
operator:

```
return Error("Identifier contains illegal character: "
 "'" + stringify(*firstInvalid) + "'");
```


- Benjamin


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


On May 2, 2017, 8 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> ---
> 
> (Updated May 2, 2017, 8 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kapil Arya.
> 
> 
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
> 
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/version.hpp 
> 7717c85b95d29cefe8f19f3cada4b7402d4d446f 
>   3rdparty/stout/tests/version_tests.cpp 
> 925f3833fd8b1732c35663df3e63c161261e9bff 
> 
> 
> Diff: https://reviews.apache.org/r/58707/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Neil Conway]

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

(Updated May 2, 2017, 8 p.m.)


Review request for mesos, Benjamin Mahler and Kapil Arya.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).

This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.


Diffs (updated)
-

  3rdparty/stout/include/stout/version.hpp 
7717c85b95d29cefe8f19f3cada4b7402d4d446f 
  3rdparty/stout/tests/version_tests.cpp 
925f3833fd8b1732c35663df3e63c161261e9bff 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Neil Conway]

> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 51-54 (original), 60-63 (patched)
> > 
> >
> > I wasn't able to see how "raw" describes what this is storing, you 
> > could call it `numericComponent` or `numericVersion`, but we could also 
> > avoid it:
> > 
> > ```
> > std::vector numeric_components =
> >   strings::split(s.substr(0, end_of_numeric_component), ".");
> > ```

It is `raw` in the sense that it hasn't been parsed yet -- i.e., it is a string 
that is supposed to contain more deeply nested structure (dot-separated 
components, each of which is a number), but that nested structure hasn't been 
validated yet.

I wasn't crazy about either `numericComponent` or `numericVersion`, because (a) 
it doesn't capture the not-parsed-yet nature of the variable (b) it is a 
string, it isn't (yet) numeric, (c) it contains multiple version 
numbers/components.

I don't like omitting the variable -- IMO the logic is easier to grasp if we 
use a separate variable.

How about `rawNumericComponents`?


> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 156-157 (patched)
> > 
> >
> > In general we try to open and close the quote on the same line if 
> > possible, as it tends to reduce the amount of times we forget to close the 
> > quote, ditto elsewhere

The annoying thing with moving the single quote to the next line is that you 
can't easily `+` two character literals together, so you end up with

```
return Error("Identifier contains illegal character: " +
 string("'") + stringify(*firstInvalid) + "'");
```

which IMO is harder to read overall.


- Neil


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


On May 1, 2017, 11:45 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> ---
> 
> (Updated May 1, 2017, 11:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kapil Arya.
> 
> 
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
> 
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/version.hpp 
> 7717c85b95d29cefe8f19f3cada4b7402d4d446f 
>   3rdparty/stout/tests/version_tests.cpp 
> 925f3833fd8b1732c35663df3e63c161261e9bff 
> 
> 
> Diff: https://reviews.apache.org/r/58707/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58889: Remove FlagsFileTest.JSONFile from Windows.

[Jeff Coffler]

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

(Updated May 2, 2017, 7 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


Changes
---

Fixed typo that Andy pointed out.


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


Repository: mesos


Description
---

This code was deprecated for Linux, and thus makes no sense
to port to Windows. This patch removes test from the Windows
platform completely.


Diffs (updated)
-

  3rdparty/stout/include/stout/flags/parse.hpp 
65edd86372596c2107e9f29cf27301e025e6620e 
  3rdparty/stout/tests/flags_tests.cpp e2681f8f68f6478d4c8a20c1e75ddb050d16b56d 


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

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


Testing
---

For Linux, all tests pass. In particular, with FlagsFileTest:

[--] 2 tests from FlagsFileTest
[ RUN  ] FlagsFileTest.JSONFile
WARNING: Logging before InitGoogleLogging() is written to STDERR
W0501 12:55:43.343416 38425 parse.hpp:101] Specifying an absolute filename to 
read a command line option out of without using 'file:// is deprecated and will 
be removed in a future release. Simply adding 'file://' to the beginning of the 
path should eliminate this warning.
[   OK ] FlagsFileTest.JSONFile (0 ms)
[ RUN  ] FlagsFileTest.FilePrefix
[   OK ] FlagsFileTest.FilePrefix (0 ms)
[--] 2 tests from FlagsFileTest (0 ms total)

For Windows, where FlagsFileTest.JSONFile is testing deprecated behavior, it's 
only appropriate for Linux. Thus, it's not run.

PS C:\mesos\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe 
--gtest_filter="FlagsFile*"
Note: Google Test filter = FlagsFile*-
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from FlagsFileTest
[ RUN  ] FlagsFileTest.FilePrefix
[   OK ] FlagsFileTest.FilePrefix (3 ms)
[--] 1 test from FlagsFileTest (3 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (7 ms total)
[  PASSED  ] 1 test.
PS C:\mesos\mesos>


Thanks,

Jeff Coffler

Re: Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58898]

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

- Mesos Reviewbot


On May 1, 2017, 11:58 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58898/
> ---
> 
> (Updated May 1, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos is now sending ShutdownFrameworkMessages to the agent for all
> non-partition-aware frameworks (including the ones that are still
> registered). This is problematic. The offer from this agent can
> still go to the same framework which can then launch new tasks.
> The agent then receives tasks of the same framework and ignores
> them because it thinks the framework is shutting down. The framework
> is not shutting down of course, so from the master and the scheduler's
> perspective the task is pending in STAGING forever until the next agent
> reregistration, which could happen much later. This commit fixes
> the problem by sending a task kill instead of ShutdownFrameworkMessage,
> when the agent re-registers after being unreachable, for non-partition
> aware framewworks.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
>   src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 
> 
> 
> Diff: https://reviews.apache.org/r/58898/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Review Request 58928: Update process tests to use a non-zero UPID.

[James Peach]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Some of the remote process tests were using a UPID with a zero
IP address field. In order to enable subsequent IP address
validation in the libprocess receiver, update these tests to
use a UPID containing the real IP address of the sender.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
921d67695bc0e4d601e9f74fbc625d69bf36ba50 


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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[James Peach]

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

(Updated May 2, 2017, 5:59 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased and split tests.


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


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


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

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


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 57964: Added a test to verify metrics when shared resources are present.

[Anindya Sinha]

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

(Updated May 2, 2017, 5:54 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added a test to verify metrics when shared resources are present.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
d42fd18fd5b36f5970e91e60b84e839aeedfc34b 


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

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

[Anindya Sinha]

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

(Updated May 2, 2017, 5:53 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The following metrics take the shared counts of shared resources into
account in determination of the actual amount of used resources:
a) `master/_used`
b) `master/_revocable_used`
c) `slave/_used`
d) `slave/_revocable_used`


Diffs (updated)
-

  src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 


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

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58708: Checked validity of master and agent version numbers on startup.

[Neil Conway]

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

(Updated May 2, 2017, 5:41 p.m.)


Review request for mesos, Benjamin Mahler and Kapil Arya.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

During startup, we now check that the compiled-in version number of the
master and agent can be parsed by stout's `Version::parse` (i.e., that
`MESOS_VERSION` is valid according to stout's extended variant of the
Semver 2.0.0 format).


Diffs (updated)
-

  src/master/main.cpp 462ed3229d15b157a92d96860503c06368ab21b7 
  src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58925: Updated runtime isolators to use new task_environment member.

[Till Toenshoff]

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

Review request for mesos, Adam B, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

see summary.


Diffs
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
ffaec5a0112db36c1e9123e361ada63a4aedf0ad 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
08350e638a0f20746e369cdc78c96126f2e1df3f 


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


Testing
---

make check and functional testing on chain end.


Thanks,

Till Toenshoff

Review Request 58923: Added new ContainerLaunchInfo task_environment.

[Till Toenshoff]

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

Review request for mesos, Adam B, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/slave/containerizer.proto 
c30b1fc659ee9b3cd343899638ced6408d8b51a2 


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


Testing
---

make check and functional testing on chain end.


Thanks,

Till Toenshoff

Review Request 58924: Updated containerizer for isolator task_environment merge.

[Till Toenshoff]

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

Review request for mesos, Adam B, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 


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


Testing
---

make check and functional testing on chain end.


Thanks,

Till Toenshoff

Re: Review Request 55897: Add support for not enforcing XFS quotas.

[James Peach]

> On May 1, 2017, 5:38 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 327 (original), 344 (patched)
> > 
> >
> > What happens if we were in accounting mode but now in enforce mode? 
> > Should we update the quota (this can potentially kill existing containers) 
> > even if it's the same? What does the posix isolator do?

The Posix isolator check the flag at the time when the accounting process 
completes. This means that processes that are over the limit before the flag 
was changed will be killed.


- James


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


On May 2, 2017, 5:23 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55897/
> ---
> 
> (Updated May 2, 2017, 5:23 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5116
> https://issues.apache.org/jira/browse/MESOS-5116
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add XFS disk isolator support for not enforcing disk quotas on
> containers. While there is a global filesystem configuration option
> to turn off quota enforcement, we should not automatically toggle
> that because we don't know why the operator might have changed that
> configuration. Instead, we just apply an unlimited (0) quota, which
> engages XFS space accounting without enforcing any limit.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 52f0459421a45b01ce38b17c689633301cd97982 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 40f1049358ad99d3f213289e36def81c580f07f3 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> eddd4c37fb42339ca21ecb392dea47acf6b277bb 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 7beb60b059910a0d4451b1ace895a35dc974a043 
> 
> 
> Diff: https://reviews.apache.org/r/55897/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55897: Add support for not enforcing XFS quotas.

[James Peach]

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

(Updated May 2, 2017, 5:23 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add XFS disk isolator support for not enforcing disk quotas on
containers. While there is a global filesystem configuration option
to turn off quota enforcement, we should not automatically toggle
that because we don't know why the operator might have changed that
configuration. Instead, we just apply an unlimited (0) quota, which
engages XFS space accounting without enforcing any limit.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
52f0459421a45b01ce38b17c689633301cd97982 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
40f1049358ad99d3f213289e36def81c580f07f3 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
eddd4c37fb42339ca21ecb392dea47acf6b277bb 
  src/tests/containerizer/xfs_quota_tests.cpp 
7beb60b059910a0d4451b1ace895a35dc974a043 


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

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


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 58709: Prevent old Mesos agents from registering or re-registering.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58897, 58707, 58708, 58709]

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

- Mesos Reviewbot


On May 1, 2017, 11:45 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58709/
> ---
> 
> (Updated May 1, 2017, 11:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kapil Arya.
> 
> 
> Bugs: MESOS-6976
> https://issues.apache.org/jira/browse/MESOS-6976
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Officially, Mesos 1.0.0 (and newer) masters do not support pre-1.0.0
> Mesos agents. However, we previously allowed old agents to register,
> which resulted in several master crashes. As a short-term solution, we
> fixed those crashes by adding backward compatibility mechanisms into the
> master, but that backward compatibility code has made the master logic
> more complicated and difficult to understand.
> 
> This commit changes the master to ignore registration attempts by Mesos
> agents that precede Mesos 1.0.0. Now that this safety check is in place,
> master logic can safely assume that all agents are running at least
> Mesos 1.0.0, which will allow several simplifications to be made.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7edf9f65f6615b67ad0663f75ea7ee72a6a558cb 
>   src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
>   src/tests/master_tests.cpp 7cb4774fae1feff007adacf6521fadde2f58bee7 
> 
> 
> Diff: https://reviews.apache.org/r/58709/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58916: Fixed a minor bug in setting the agent's `totalResources`.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On May 2, 2017, 1:26 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58916/
> ---
> 
> (Updated May 2, 2017, 1:26 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the following branch is taken, `totalResources` would be set
> to `resources.get()` which is not equal to `info.resources()`.
> 
> ```cpp
>   if (HookManager::hooksAvailable()) {
> info.mutable_resources()->CopyFrom(
> HookManager::slaveResourcesDecorator(info));
>   }
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
> 
> 
> Diff: https://reviews.apache.org/r/58916/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 28, 2017, 8:49 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1484 (patched)
> > 
> >
> > Curious if we want to allow isolators to override this behavior for 
> > debug containers just like non-debug containers.

A good question. My first thought is not, we would like DEBUG containers to 
have the same working directory as their parents. On the other side, I don't 
have enough context to understand or envision cases where an isolator would set 
work dir to different values for a container and its children.


- Alexander


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


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
> https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58821/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 58916: Fixed a minor bug in setting the agent's `totalResources`.

[Michael Park]

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

Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

If the following branch is taken, `totalResources` would be set
to `resources.get()` which is not equal to `info.resources()`.

```cpp
  if (HookManager::hooksAvailable()) {
info.mutable_resources()->CopyFrom(
HookManager::slaveResourcesDecorator(info));
  }
```


Diffs
-

  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 


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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 58892: Added C++11 scoped enumeration to style guide.

[Michael Park]

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




docs/c++-style-guide.md
Lines 649 (patched)


Maybe just mention `enum class` to eliminate confusion as to which one 
should be used?


- Michael Park


On May 1, 2017, 2:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58892/
> ---
> 
> (Updated May 1, 2017, 2:07 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We've been using it for new code now.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md ccb81f48e2edc9c1e7c328cc26e44d658b4c43b4 
> 
> 
> Diff: https://reviews.apache.org/r/58892/diff/1/
> 
> 
> Testing
> ---
> 
> Markdown viewer.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 58587: Clarified comments about resource operations through operator API.

[Michael Park]

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




src/master/http.cpp
Lines 4870-4872 (patched)


Thanks for adding this! This is the issue that I was considering filing a 
JIRA ticket for.



src/master/http.cpp
Lines 4884-4889 (patched)


I'm not quite following this example. Specifically, I don't get what "a 
persistent volume can be destroyed if it is not allocated" means..


- Michael Park


On May 1, 2017, 2:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58587/
> ---
> 
> (Updated May 1, 2017, 2:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Seems like the comments about "virtually always win the race against
> 'allocate'" could be made more precise in helping people understand
> how it works.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
> 
> 
> Diff: https://reviews.apache.org/r/58587/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>