Re: Review Request 58725: Fetching docker image manifests with 'Accept' header.

[Chun-Hung Hsiao]

> On April 27, 2017, 12:46 a.m., Jie Yu wrote:
> > Do you know if an old registry (< 2.3) will be OK with this header that it 
> > does not understand? Can you confirm?
> 
> Ilya Pronin wrote:
> Shall we include all content types that we currently support? (Can be 
> splitted into multiple `Accept` fields)
> ```
> Accept: application/vnd.docker.distribution.manifest.v1+json,
> application/vnd.docker.distribution.manifest.v1+prettyjws,
> application/json
> ```

I tried but Amazon ECR would reject the header because it's check uses the 
following regex: `\w{1,127}\/[-+.\w]{1,127}]`, so only one MIME type is allowed 
for ECR.


- Chun-Hung


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


On April 26, 2017, 10:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58725/
> ---
> 
> (Updated April 26, 2017, 10:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7427
> https://issues.apache.org/jira/browse/MESOS-7427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Accept: application/vnd.docker.distribution.manifest.v1+json'
> to the headers of HTTP requests for fetching manifests from any Docker
> registry. Some registry services (e.g., Amazon ECR) check the 'Accept'
> field strictly and reject the requests if it is not specified.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 1c6ab929deacfc29aa6b4f1df04c2b9782044a90 
> 
> 
> Diff: https://reviews.apache.org/r/58725/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a local docker private registry and an Amazon ECR 
> repository.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58725: Fetching docker image manifests with 'Accept' header.

[Chun-Hung Hsiao]

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

(Updated May 1, 2017, 5:24 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

More manual tests


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


Repository: mesos


Description
---

Added 'Accept: application/vnd.docker.distribution.manifest.v1+json'
to the headers of HTTP requests for fetching manifests from any Docker
registry. Some registry services (e.g., Amazon ECR) check the 'Accept'
field strictly and reject the requests if it is not specified.


Diffs
-

  src/uri/fetchers/docker.cpp 1c6ab929deacfc29aa6b4f1df04c2b9782044a90 


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


Testing (updated)
---

sudo make check
Manually tested on the following registries:
1. Local docker private registries with the following version:
   2.0.1, 2.1.1, 2.2.1, 2.3.1, 2.4.1, 2.5.1, 2.6.1
2. Amazon ECR repository


Thanks,

Chun-Hung Hsiao

Re: Review Request 58766: Fixed coding style issues in ProvisionerDockerBackendTest.

[Chun-Hung Hsiao]

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

(Updated May 1, 2017, 6:14 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Neil Conway.


Changes
---

Addressed Jie's comment.


Summary (updated)
-

Fixed coding style issues in ProvisionerDockerBackendTest.


Repository: mesos


Description (updated)
---

Replaced a string concatenation with path::join() and
a shell echo command with os::write() in
ProvisionerDockerBackendTest.ROOT_INTERNET_CURL_Overwrite.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
52e57cef81f79ebae4d5305708929396c5d75680 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 58725: Fetching docker image manifests with 'Accept' header.

[Chun-Hung Hsiao]

> On April 27, 2017, 12:46 a.m., Jie Yu wrote:
> > Do you know if an old registry (< 2.3) will be OK with this header that it 
> > does not understand? Can you confirm?
> 
> Ilya Pronin wrote:
> Shall we include all content types that we currently support? (Can be 
> splitted into multiple `Accept` fields)
> ```
> Accept: application/vnd.docker.distribution.manifest.v1+json,
> application/vnd.docker.distribution.manifest.v1+prettyjws,
> application/json
> ```
> 
> Chun-Hung Hsiao wrote:
> I tried but Amazon ECR would reject the header because it's check uses 
> the following regex: `\w{1,127}\/[-+.\w]{1,127}]`, so only one MIME type is 
> allowed for ECR.

Also, setting `Accept` to just `application/json` would make ECR to return a 
Schema 2 manifest instead of a Schema 1 one.


- Chun-Hung


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


On May 1, 2017, 5:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58725/
> ---
> 
> (Updated May 1, 2017, 5:24 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7427
> https://issues.apache.org/jira/browse/MESOS-7427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Accept: application/vnd.docker.distribution.manifest.v1+json'
> to the headers of HTTP requests for fetching manifests from any Docker
> registry. Some registry services (e.g., Amazon ECR) check the 'Accept'
> field strictly and reject the requests if it is not specified.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 1c6ab929deacfc29aa6b4f1df04c2b9782044a90 
> 
> 
> Diff: https://reviews.apache.org/r/58725/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on the following registries:
> 1. Local docker private registries with the following version:
>2.0.1, 2.1.1, 2.2.1, 2.3.1, 2.4.1, 2.5.1, 2.6.1
> 2. Amazon ECR repository
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58725: Fetching docker image manifests with 'Accept' header.

[Chun-Hung Hsiao]

> On April 27, 2017, 12:46 a.m., Jie Yu wrote:
> > Do you know if an old registry (< 2.3) will be OK with this header that it 
> > does not understand? Can you confirm?
> 
> Ilya Pronin wrote:
> Shall we include all content types that we currently support? (Can be 
> splitted into multiple `Accept` fields)
> ```
> Accept: application/vnd.docker.distribution.manifest.v1+json,
> application/vnd.docker.distribution.manifest.v1+prettyjws,
> application/json
> ```
> 
> Chun-Hung Hsiao wrote:
> I tried but Amazon ECR would reject the header because it's check uses 
> the following regex: `\w{1,127}\/[-+.\w]{1,127}]`, so only one MIME type is 
> allowed for ECR.
> 
> Chun-Hung Hsiao wrote:
> Also, setting `Accept` to just `application/json` would make ECR to 
> return a Schema 2 manifest instead of a Schema 1 one.

For ECR, if there is a signed manifest, requesting 
`application/vnd.docker.distribution.manifest.v1+json` will receive the signed 
one.


- Chun-Hung


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


On May 1, 2017, 5:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58725/
> ---
> 
> (Updated May 1, 2017, 5:24 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7427
> https://issues.apache.org/jira/browse/MESOS-7427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Accept: application/vnd.docker.distribution.manifest.v1+json'
> to the headers of HTTP requests for fetching manifests from any Docker
> registry. Some registry services (e.g., Amazon ECR) check the 'Accept'
> field strictly and reject the requests if it is not specified.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 1c6ab929deacfc29aa6b4f1df04c2b9782044a90 
> 
> 
> Diff: https://reviews.apache.org/r/58725/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on the following registries:
> 1. Local docker private registries with the following version:
>2.0.1, 2.1.1, 2.2.1, 2.3.1, 2.4.1, 2.5.1, 2.6.1
> 2. Amazon ECR repository
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58725: Fetching docker image manifests with 'Accept' header.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58753, 58725]

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, 5:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58725/
> ---
> 
> (Updated May 1, 2017, 5:24 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7427
> https://issues.apache.org/jira/browse/MESOS-7427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Accept: application/vnd.docker.distribution.manifest.v1+json'
> to the headers of HTTP requests for fetching manifests from any Docker
> registry. Some registry services (e.g., Amazon ECR) check the 'Accept'
> field strictly and reject the requests if it is not specified.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 1c6ab929deacfc29aa6b4f1df04c2b9782044a90 
> 
> 
> Diff: https://reviews.apache.org/r/58725/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on the following registries:
> 1. Local docker private registries with the following version:
>2.0.1, 2.1.1, 2.2.1, 2.3.1, 2.4.1, 2.5.1, 2.6.1
> 2. Amazon ECR repository
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


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 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs
-

  src/uri/fetchers/docker.cpp 44169bf5f22f0ffd9fad7bb3b8f7d2a4989c6415 


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


Testing
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR


Thanks,

Chun-Hung Hsiao

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

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


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.

[--] 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)


Thanks,

Jeff Coffler

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 1, 2017, 8 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/1/


Testing (updated)
---

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 58874: Stripped spaces between directory elements in WebUI.

[Tomasz Janiszewski]

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


Fix it, then Ship it!




Nice usability improvement.


src/webui/master/static/browse.html
Line 17 (original), 17 (patched)


Could you keep original formatting?



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.


- Tomasz Janiszewski


On Kwi 30, 2017, 4:29 rano, haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58874/
> ---
> 
> (Updated Kwi 30, 2017, 4:29 rano)
> 
> 
> 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 58766: Fixed coding style issues in ProvisionerDockerBackendTest.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58764, 58766]

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, 6:14 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58766/
> ---
> 
> (Updated May 1, 2017, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced a string concatenation with path::join() and
> a shell echo command with os::write() in
> ProvisionerDockerBackendTest.ROOT_INTERNET_CURL_Overwrite.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 52e57cef81f79ebae4d5305708929396c5d75680 
> 
> 
> Diff: https://reviews.apache.org/r/58766/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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 1, 2017, 8:42 p.m.)


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


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
-

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


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


Testing
---

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


Thanks,

Jacob Janco

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

[Tomasz Janiszewski]

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


Fix it, then Ship it!




I tested it and it fixes the issue. I think code need some comments, it's 
simple but not obvious.


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



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?


- Tomasz Janiszewski


On Kwi 29, 2017, 6:22 po południu, haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> ---
> 
> (Updated Kwi 29, 2017, 6:22 po południu)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured sandbox URI request reroute after fetched `$scope.state`.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> a021962573d452de1581e6a7717016eac7d0cd85 
> 
> 
> Diff: https://reviews.apache.org/r/58872/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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 1, 2017, 9:01 p.m.)


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


Changes
---

I understand what Joe was asking for, and resolved that in a second commit. 
Already posted for review.


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

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


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

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

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

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

[Jiang Yan Xu]

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

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 58759: Introduced SecretFetcher module interface.

[Vinod Kone]

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




include/mesos/secret/secretfetcher.hpp
Lines 27 (patched)


Should this be just called `Fetcher` since it is in the `secret` namespace 
already? Looks like this is how URI, APPC etc fetchers are named.


- Vinod Kone


On April 28, 2017, 6:50 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58759/
> ---
> 
> (Updated April 28, 2017, 6:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced SecretFetcher module interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/secretfetcher.hpp PRE-CREATION 
>   include/mesos/secret/secretfetcher.hpp PRE-CREATION 
>   src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
>   src/module/manager.cpp 7d875fcb7fcec0d57274e644b0a3b67b333ac193 
> 
> 
> Diff: https://reviews.apache.org/r/58759/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 58760: Added default secret fetcher module.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On April 28, 2017, 6:50 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58760/
> ---
> 
> (Updated April 28, 2017, 6:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default module doesn't attempt to interpret Secret::reference and simply 
> returns Secret::value.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
>   src/secret/fetcher.hpp PRE-CREATION 
>   src/secret/fetcher.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58760/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 58761: Added --secret-fetcher flag for agent.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/main.cpp
Line 234 (original), 243-245 (patched)


why is this mentioned twice? what's the difference between "Secret fetcher" 
and "SecretFetcher" ?


- Vinod Kone


On April 28, 2017, 6:51 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58761/
> ---
> 
> (Updated April 28, 2017, 6:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7418
> https://issues.apache.org/jira/browse/MESOS-7418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The flag accepts a module name. The module should be specified via 
> `--modules` or `--modules_dir` flag.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 
>   src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94 
>   src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173 
>   src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
> 
> 
> Diff: https://reviews.apache.org/r/58761/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

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

[Andrew Schwartzmeyer]

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




3rdparty/stout/include/stout/flags/parse.hpp
Lines 98 (patched)


s/Noate/NOTE:


- Andrew Schwartzmeyer


On May 1, 2017, 8 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58889/
> ---
> 
> (Updated May 1, 2017, 8 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/1/
> 
> 
> 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 57963: Metrics for used resources should incorporate shared resources.

[Jiang Yan Xu]

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


Fix it, then Ship it!





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


s/same/the same/ I forgot to say "all ocurrences".



src/slave/slave.cpp
Line 6743 (original), 6743 (patched)


Perhaps I am pedantic but I think s/used/Resources arithmetic/ here is more 
clear. When I read "We use `used`" my immediate reaction was "instead of what?" 
because `used` is the only variable here. The comment is basically to remind 
the reader of the "magic" of Resources::+= so perhaps just directly say it?

We could use the same wording for the `slaveUsed` case above so all of 
these comments are consistent (which was my original suggestion).


- Jiang Yan Xu


On April 22, 2017, 10 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated April 22, 2017, 10 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> 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
> -
> 
>   src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
>   src/slave/slave.cpp 4ff522e75bc8de34fe2e7720bdd8ce3d32cbf803 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58778: Supported GCE container registry.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58753, 58725, 58778]

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, 7:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58778/
> ---
> 
> (Updated May 1, 2017, 7:24 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7431
> https://issues.apache.org/jira/browse/MESOS-7431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Certain registries, such as GCE registry, reply 403 instead of 401 for
> unauthorized requests. When fetching image manifests and blobs, instead
> of sending out unauthorized requests first and waiting for a possible
> 401, we should always look up the docker config and send requests with
> basic authorization when possible.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 44169bf5f22f0ffd9fad7bb3b8f7d2a4989c6415 
> 
> 
> Diff: https://reviews.apache.org/r/58778/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (covers all supported public registries)
> Manually tested on the following private registries:
> 1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
> 2. Amazon ECR
> 3. Google GCR
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/persistent_volume_tests.cpp
Lines 1175 (patched)


s/use shared persistent volumes to launch/can use the same shared 
persistent volume to launch/ 

This pairs with the word `verifies`?

In the next sentence, s/It verifies/It also verifies/ ?


- Jiang Yan Xu


On April 22, 2017, 10 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57964/
> ---
> 
> (Updated April 22, 2017, 10 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> 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
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 3eb7afe3de8e72ffb6502dfe12ef37ccd4ca2125 
> 
> 
> Diff: https://reviews.apache.org/r/57964/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

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

[Benjamin Mahler]

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



Thanks for tackling this!


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?


- Benjamin Mahler


On April 29, 2017, 6:22 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> ---
> 
> (Updated April 29, 2017, 6:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured sandbox URI request reroute after fetched `$scope.state`.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> a021962573d452de1581e6a7717016eac7d0cd85 
> 
> 
> Diff: https://reviews.apache.org/r/58872/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Benjamin Mahler]

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



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?

- Benjamin Mahler


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 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 66-71 (original), 139-182 (patched)
> > 
> >
> > These are made public but I can't see how these would be useful to any 
> > callers, could we just inline lambdas them in the parse function for now 
> > until these prove needed by some callers?

I marked these as `private`, but I didn't make them lambdas yet: personally, I 
find large function bodies containing multiple nested lambdas hard to read (it 
also makes me think through whether the lambda captures any variables, etc.). 
Happy to discuss further if you'd prefer lambdas here.


- Neil


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


On April 26, 2017, 5:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> ---
> 
> (Updated April 26, 2017, 5:35 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 
> 724ed2292fdd3c5f4c98facf82260078b66a0e97 
> 
> 
> Diff: https://reviews.apache.org/r/58707/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58517: Update SlaveRecoveryTest to send libprocess messages from a real UPID.

[Benjamin Mahler]

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


Ship it!




This review seems to suggest that `process::post` without a `from` address 
shouldn't be exposed? Do you have any thoughts on this?

- Benjamin Mahler


On May 1, 2017, 11:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58517/
> ---
> 
> (Updated May 1, 2017, 11:41 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update SlaveRecoveryTest to send libprocess messages from a real UPID.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 53f33a2b0411c8158326074ce043c7b1dbeef5b4 
> 
> 
> Diff: https://reviews.apache.org/r/58517/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 58897: Added a test case for numify in stout.

[Neil Conway]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added a test case for numify in stout.


Diffs
-

  3rdparty/stout/tests/numify_tests.cpp 
e28384a45109e8fcb1ceefa6163f13d188e3cd0e 


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


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 1, 2017, 11:45 p.m.)


Review request for mesos, Benjamin Mahler and Kapil Arya.


Changes
---

Checkpoint addressing 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/4/

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


Testing
---

`make check`


Thanks,

Neil Conway

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

[Neil Conway]

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


Changes
---

Address review comments.


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

  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/

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58778: Supported GCE container registry.

[Chun-Hung Hsiao]

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

(Updated May 1, 2017, 11:47 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

More manual tests.


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


Repository: mesos


Description
---

Certain registries, such as GCE registry, reply 403 instead of 401 for
unauthorized requests. When fetching image manifests and blobs, instead
of sending out unauthorized requests first and waiting for a possible
401, we should always look up the docker config and send requests with
basic authorization when possible.


Diffs
-

  src/uri/fetchers/docker.cpp 44169bf5f22f0ffd9fad7bb3b8f7d2a4989c6415 


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


Testing (updated)
---

sudo make check (covers all supported public registries)
Manually tested on the following private registries:
1. Local registry (2.0.1, 2.1.1, 2.2.1, ..., 2.6.1)
2. Amazon ECR
3. Google GCR
4. JFrog SaaS
5. Local Nexus registry 3.3.1


Thanks,

Chun-Hung Hsiao

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

[Megha Sharma]

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

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


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 58517: Update SlaveRecoveryTest to send libprocess messages from a real UPID.

[Benjamin Mahler]

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




src/tests/slave_recovery_tests.cpp
Lines 2348-2349 (original), 2348-2350 (patched)


Per offline discussion, will omit this latter line since it seems to be 
confusing.


- Benjamin Mahler


On May 1, 2017, 11:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58517/
> ---
> 
> (Updated May 1, 2017, 11:41 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update SlaveRecoveryTest to send libprocess messages from a real UPID.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 53f33a2b0411c8158326074ce043c7b1dbeef5b4 
> 
> 
> Diff: https://reviews.apache.org/r/58517/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Megha Sharma]

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

  src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
  src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 


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

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


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 55362: Support asynchronous fetching of URIs.

[Megha Sharma]

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

(Updated May 2, 2017, 12:08 a.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Mesos fetcher fetches the URIs specified in CommandInfo serially,
one after the other. If the task has too many URIs to be fetched
then it delays the execution of the task.


Diffs (updated)
-

  src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 


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

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


Testing
---

make check on macOS.


Thanks,

Megha Sharma

Re: Review Request 58762: Use SecretFetcher to fetch secret executor environment.

[Vinod Kone]

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




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


s/secret resolution in executor environment/calculating executor 
environment/ ?

I think the secret fetching part should be internal to the 
`executorEnvironment` function.



src/slave/slave.cpp
Line 7625 (original), 7637 (patched)


CHECK_SOME(secretFetcher);



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


Ouch. Lets not do await please. We need to make this asynchronous.



src/slave/slave.cpp
Lines 7665-7666 (patched)


I think these should return an error instead of crashing the agent.



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


Ditto. No await please.


- Vinod Kone


On April 28, 2017, 6:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58762/
> ---
> 
> (Updated April 28, 2017, 6:54 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7419
> https://issues.apache.org/jira/browse/MESOS-7419
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Secrets specified in the executor environment variables are resolved using a 
> secret-fetcher module. If no secret-fetcher module is specified, an internal 
> default implementation is used to resolve the secrets.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4 
>   src/tests/hook_tests.cpp 02d8f800c3eb9b1e617a14c78c2ef1e45d1c72bb 
> 
> 
> Diff: https://reviews.apache.org/r/58762/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

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

[Benjamin Mahler]

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



Can you pull the test changes out into a separate patch? Overall looks pretty 
good, just some details to sort through first.


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


How about `validate_peer_address`? Pin to me seems to suggest something 
that was floating across multiple things is now fixed to specific things.



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


How about "Validate that the ...", Force seems to suggest we can force it 
to happen?



3rdparty/libprocess/src/process.cpp
Lines 220-221 (patched)


Is it possible to elaborate on which configurations this will break? I'm 
pretty naive here and don't know, so would love to have a more comprehensive 
explanation. :)



3rdparty/libprocess/src/process.cpp
Lines 217-220 (original), 225-229 (patched)


Generally it seems that we should have global access to the flags from 
within libprocess, or if not the flags, the runtime properties that are deduced 
from the flags (the former would be simpler for now).

In terms of impacting this patch, it would mean that we don't need to pass 
the `validate_peer_address` bit around?



3rdparty/libprocess/src/process.cpp
Lines 480-492 (patched)


I'm wondering if we can eliminate the need for this via global flag access 
and peer caching in the Socket implementation, see other comments.



3rdparty/libprocess/src/process.cpp
Line 942 (original), 965 (patched)


Per our offline discussion, could we acheive the elimination of 
`getpeername`-per-read by having the `Socket` perform this optimization for a 
connected socket? That would also avoid the need to carry the peer around.



3rdparty/libprocess/src/process.cpp
Lines 2878-2879 (patched)


We could refer to the flag help for examples?



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


Hm.. is this the best error code for this? It seems like a client error 
rather than a server error?



3rdparty/libprocess/src/process.cpp
Lines 2883-2884 (patched)


How about:

UPID IP address validation failed: Message from X was sent from IP Y.


- Benjamin Mahler


On May 1, 2017, 11:40 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 1, 2017, 11:40 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
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 92efa915414c2a38b18de99858c66b63e757f63c 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 921d67695bc0e4d601e9f74fbc625d69bf36ba50 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/2/
> 
> 
> 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 58897: Added a test case for numify in stout.

[Benjamin Mahler]

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


Ship it!




Good to know!

- Benjamin Mahler


On May 1, 2017, 11:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58897/
> ---
> 
> (Updated May 1, 2017, 11:44 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test case for numify in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/numify_tests.cpp 
> e28384a45109e8fcb1ceefa6163f13d188e3cd0e 
> 
> 
> Diff: https://reviews.apache.org/r/58897/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 58900: Changed the default fetcher cache directory.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

The fetcher cache directory was historically located (by default)
in `/tmp/mesos/fetch`.  The agent flag `--fetcher_cache_dir` could
be used to change this value.

The fetcher would create a subdirectory underneath `/tmp/mesos/fetch`
for each `SlaveID`.  This was done because multiple agents can run on
the same node.  If all the agents use the same default fetcher cache
directory, they will collide and cause unpredictable results.
As a result, the `SlaveID` needed to be passed into the fetcher
after the agent recovers and/or registers with the master, because
that is when the `SlaveID` is determined.

This changes the default fetcher cache directory to
`/tmp/mesos/fetch/XX`, where the 6 X's are replaced by `::mkdtemp`.
The `SlaveID` subdirectory has also been removed.

This change, while techically a breaking change, is safe because of
how the fetcher uses this directory.  Upon starting up, the fetcher
"recovers" by clearing this directory.  By using a temporary directory,
we similarly get an empty fetcher cache upon startup.

This change will only cause breakages if multiple agents are run
with the same `--fetcher_cache_dir`.  In this case, each agent
will delete the fetcher caches of all the other agents.


Diffs
-

  src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043 
  src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611 
  src/slave/flags.cpp ed99fadbf1aa91f7f0a57c9fb351c0247a40c6f4 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58901: Removed `Fetcher::recover` static method.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

With the removal of the `SlaveID` field in the fetcher's methods,
it is no longer necessary to pass in the `SlaveID` or agent Flags
at agent recovery time.  Instead, the flags can be passed in during
the fetcher's construction.

Similarly, the fetcher's "recovery" (clearing the fetcher cache)
can be done immediately upon construction, which simplifies the
code slightly.


Diffs
-

  src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 
  src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043 
  src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611 
  src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52 
  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58899: Combined containerizer interface's launch methods.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

When nested container support was added, we added a separate `launch`
path in the containerizer because nested containers do not need
an explicit TaskInfo/ExecutorInfo.  Nested containers basically
only need the CommandInfo and ContainerInfo.

This commit combines the two launch methods by replacing most of the
"Infos" (Task, Executor, Command, Container) with a `ContainerConfig`
argument, which may contain multiple combinations of the "Infos".

The goal is to support three launch paths for containers:
  1) When the `ContainerConfig` contains a TaskInfo/ExecutorInfo,
 launch a task or executor.
  2) When the `ContainerID` has a parent, launch a nested container.
  3) (Not implemented yet) When there is no TaskInfo/ExecutorInfo or
 parent container, launch a standalone container.
 
There are two other notable changes to the interface:
  * The `SlaveID` field has been removed entirely.  The code that
requires this (in the fetcher and Docker containerizer) will be
addressed in a separate commit.
  * The `checkpoint` bool has been replaced by an Option,
which contains the path that should be used for checkpointing.
This path includes the filename.
This is also one of the reasons why `SlaveID` was an argument.


Diffs
-

  src/slave/containerizer/containerizer.hpp 
4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58902: Changed containerizer->launch callsites to new interface.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

As part of combining the two `containerizer->launch` code paths,
callers now need to construct the `ContainerConfig` (instead of
passing some fields that are then copied into a `ContainerConfig`).


Diffs
-

  src/slave/http.cpp 2ce345d9958a867ece835e47287215cefd561abf 
  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58904: Combined launch paths in ComposingContainerizer.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This eliminates one of the entrypoints for launching a container
in the `ComposingContainerizer`.  Nested and non-nested containers
are still launched the same way, where nested containers get launched
using their root container's containerizer.


Diffs
-

  src/slave/containerizer/composing.hpp 
8e04bfeb76607e87ee336f4a7f519eac08b7d44c 
  src/slave/containerizer/composing.cpp 
0b6c76b3d081d86df81a6062ae7a191ba8dadfde 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58903: Combined Mesos containerizer's launch methods.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This simplifies the container launch path by removing combining
the nested and non-nested container code paths into one.

The Mesos containerizer was originally translating the two
`containerizer->launch` entrypoints into a common method (also
called `launch`).  The previous commits moved this translation
logic into the caller (i.e. the Agent).

The end result has some slight changes:
  * It is now possible for the Agent to specify some more combinations
of `ContainerConfig`.  For example, specifying a TaskInfo with
a DEBUG-class container.  Or a nested container with Resources.
We may need to add extra validation around this.
  * The `bool checkpoint` argument was replaced with a `Option`
which optionally contains an absolute path.  This allows us to
remove the `SlaveID` field.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58905: Changed naming of Docker containers to the pre-0.23 scheme.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

The choice of naming scheme is cosmetic.  All versions of the Docker
containerizer will consider containers starting with "mesos-"
as containers started by Mesos.  The Docker containerizer does not
consider the `SlaveID` at all when recovering.


Diffs
-

  src/slave/containerizer/docker.hpp 44efa44586455df416fc9cdc51c92bd9027f24c3 
  src/slave/containerizer/docker.cpp 2480f595bee032ea399d642653d618472db0b7a7 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58906: Removed extranous sandbox initialization in Docker containerizer.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

The Docker containerizer will create a stdout/stderr file as well
`chown` the sandbox directory to the user specified at container
creation time.  This logic is already performed elsewhere.

The stdout/stderr files will either be created prior to the container
launch via the mesos-fetcher, or during launch via the ContainerLogger.

The sandbox directory will be `chown`d by the Agent when it
starts to launch the executor.


Diffs
-

  src/slave/containerizer/docker.cpp 2480f595bee032ea399d642653d618472db0b7a7 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58907: Refactored Docker containerizer launch path per interface changes.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This continues the containerizer interface change that replaced
most "Infos" (Task, Executor, Command, Container) with a single
`ContainerConfig` and combined the nested/non-nested container
launch paths.

Notably, this commit also changes the fields stored in the Docker
containerizer to match the new interface.  The somewhat ambiguously
named `directory` field has been renamed to `containerWorkDir`.
And the copy of `Slave::flags` in each container has been removed
because it is not used.


Diffs
-

  src/slave/containerizer/docker.hpp 44efa44586455df416fc9cdc51c92bd9027f24c3 
  src/slave/containerizer/docker.cpp 2480f595bee032ea399d642653d618472db0b7a7 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58908: Updated test mocks per interface changes.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


Bugs: MESOS-7304 and MESOS-7449
https://issues.apache.org/jira/browse/MESOS-7304
https://issues.apache.org/jira/browse/MESOS-7449


Repository: mesos


Description
---

This updates the mocks for the containerizers and fetcher after
the interface changes.  For the containerizers, the two launch methods
have been combined into a single method.

The fetcher now has an argument in its constructor and no longer
takes some arguments (SlaveID and Flags) in its methods.

Launching nested containers via the test containerizer was
originally a no-op, so similar behavior is implemented in the
combined method.


Diffs
-

  src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f 
  src/tests/containerizer.hpp 63fe2366d5fe79c71cbe62813a9bb0b830e70057 
  src/tests/containerizer.cpp 548da3a8757c9ad3ab115c6b91f8a1f2f5e37144 
  src/tests/containerizer/mock_containerizer.hpp 
ca0ae053b1eed01d6c04f581cd08485beec0c5fb 
  src/tests/mesos.hpp 3c57f25ff0bec55582b8a7decef53be61a9ec6f4 
  src/tests/mesos.cpp a79ec62d6b8354a97a50533dc66e04c1afc4bef6 
  src/tests/mock_docker.hpp f58211d93e3231bfef7afa9e495930a70aae9f2b 
  src/tests/mock_docker.cpp 81a14ca346f0fbe6458e03b06932e1ad00a4a238 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58909: Added test helpers to tranlate to ContainerConfig.

[Joseph Wu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This adds a helper that emulates the logic in the Agent's launch path.
Now that the Containerizer takes a ContainerConfig instead of
a TaskInfo/ExecutorInfo/Directory/SlaveID/User, tests that call
the containerizer directly will need to translate to
ContainerConfig.


Diffs
-

  src/tests/mesos.hpp 3c57f25ff0bec55582b8a7decef53be61a9ec6f4 


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


Testing
---

See last patch in chain.


Thanks,

Joseph Wu

Review Request 58910: [WIP] Updated all tests that use Containerizer::launch(...).

[Joseph Wu]

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

Review request for mesos and Jie Yu.


Bugs: MESOS-7304 and MESOS-7449
https://issues.apache.org/jira/browse/MESOS-7304
https://issues.apache.org/jira/browse/MESOS-7449


Repository: mesos


Description
---

This sweeps through the unit tests and translates all existing
tests that use the two variants of Containerizer::launch(...)
(i.e. nested and non-nested) and translates the arguments to the
new interface.


Diffs
-

  src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
  src/tests/command_executor_tests.cpp da4b653303d436309f6cb190ad6b600b8d934678 
  src/tests/container_logger_tests.cpp 28436b6b67c1251d877064751e695c6696725a23 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
4e1d027bde7f0ab51e778bfd3bac8797fc7f7f1b 
  src/tests/containerizer/cni_isolator_tests.cpp 
565e58ae75e918453e4386f5e35a5a844a8b15f8 
  src/tests/containerizer/composing_containerizer_tests.cpp 
d7fd6216080f2f437503b898bff6b1046317933e 
  src/tests/containerizer/cpu_isolator_tests.cpp 
3c6f7481de2d6d1f6abf9850bd33cea44931480a 
  src/tests/containerizer/docker_containerizer_tests.cpp 
4eef399b05f6e35d75c7c23992f0ccda04576277 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
b47a6b5081a63ac474ac4634701b1a572eb58137 
  src/tests/containerizer/io_switchboard_tests.cpp 
b19961ee91af3c174d4377c2514465d6c99cf331 
  src/tests/containerizer/isolator_tests.cpp 
355e15ff69ca6bdd94821f6566fd09a280d03b47 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
70a0dce0f0166fcb3c15a03a89151152954abbac 
  src/tests/containerizer/memory_isolator_tests.cpp 
0df4aa892b531eb305eb3012862ad25527344ab6 
  src/tests/containerizer/memory_pressure_tests.cpp 
c4ad779906ee31d07fdd8a6fc0e51e1edd1cbe85 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 
  src/tests/containerizer/port_mapping_tests.cpp 
a528382e8b4831b9c7e8dcc877a5e242909f0cd5 
  src/tests/containerizer/volume_image_isolator_tests.cpp 
ad1884459e87ece767e1019adcf702a2f2ff8f1c 
  src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
5f4e382baf80439b9a61f22c1912cadeba1109b7 
  src/tests/containerizer/xfs_quota_tests.cpp 
7beb60b059910a0d4451b1ace895a35dc974a043 
  src/tests/disk_quota_tests.cpp f758bee07c448afe09dc380e99c4a81e2dd9f650 
  src/tests/fetcher_tests.cpp 27ea7244c713934ef5a721ec133804a006dcb26e 
  src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad 
  src/tests/hook_tests.cpp 02d8f800c3eb9b1e617a14c78c2ef1e45d1c72bb 
  src/tests/master_tests.cpp 7cb4774fae1feff007adacf6521fadde2f58bee7 
  src/tests/slave_recovery_tests.cpp 53f33a2b0411c8158326074ce043c7b1dbeef5b4 
  src/tests/slave_tests.cpp 8c97dc6d088708d301dc3ccf90d413fd785b782f 


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


Testing
---

WIP.

Doesn't actually build yet (more test changes needed).


Thanks,

Joseph Wu