Re: Review Request 71441: Fixed Docker registry malformed URI contruction.

[Qian Zhang]

> On Sept. 16, 2019, 1:24 p.m., Qian Zhang wrote:
> > In the description of this patch, I see you mentioned we only have this 
> > issue with a non-default Docker registry, but that seems not what I found. 
> > I started Mesos agent without setting the flag `--docker_registry` (so it 
> > just took the default value: `https://registry-1.docker.io`) and then 
> > launched a container with an image named `zhq527725/whiteout:latest`, and 
> > then I see:
> > > Pulling image 'zhq527725/whiteout:latest' from 
> > > 'docker-manifest://registry-1.docker.io:443zhq527725/whiteout?latest#https'
> > >  to '/opt/qzhang/mesos/store/docker/staging/pKccqL'
> > 
> > As you see, the separator between the hostname and the path was missed as 
> > well, and I found the container can be launched successfully. So what is 
> > actually the issue? Just the malformed registry URLs in the above message? 
> > If I read MESOS-9963 correctly, the issue should be launching container 
> > fails, right?
> 
> James Peach wrote:
> In my testing, the malformed URL is used to pull the images, and that 
> fails. Is there a code path I'm missing?
> 
> Qian Zhang wrote:
> Can you please let me know the detailed error message in your testing?
> 
> IIUC, the issue (missing separator) is in the method `ostream& 
> operator<<(ostream& stream, const URI& uri)`, and that method is used to 
> print the message `Pulling image ...` but not used when actually fetching 
> image, right?
> 
> James Peach wrote:
> The curl fetcher uses it via 
> [stringify](https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L129).
> 
> Qian Zhang wrote:
> I think you are talking about [this 
> code](https://github.com/apache/mesos/blob/1.9.0/src/uri/fetchers/docker.cpp#L235),
>  right? However the URI used in that code is not the malformed URI that you 
> mentioned in this patch, it is actually re-constructed from the malformed URI 
> (see `DockerFetcherPluginProcess::getManifestUri` and 
> `DockerFetcherPluginProcess::getBlobUri`), so even we see the malformed URL 
> in the message like this 
> `"docker-manifest://registry-1.docker.io:443zhq527725/whiteout?latest#https"`,
>  the URI used by curl is still correct, like: 
> `"https://registry-1.docker.io:443/v2/zhq527725/whiteout/manifests/latest"`.
> 
> James Peach wrote:
> Ah I see. The Docker puller makes sure to place a leading '/' in the 
> path. I asked the relevant team to re-test and they confirmed that this is 
> purely a cosmetic issue.
> 
> I still think that this change is correct, but I'll update the commit 
> message :)

Thanks, please also update the description of 
https://issues.apache.org/jira/browse/MESOS-9963 .


- Qian


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


On Sept. 6, 2019, 2:20 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71441/
> ---
> 
> (Updated Sept. 6, 2019, 2:20 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-9963
> https://issues.apache.org/jira/browse/MESOS-9963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a non-default Docker registry is used for container image, the Docker
> puller would omit the separator between the hostname and the path when
> constructing the final URI.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_spec_tests.cpp 
> 4be0716895132e0406299f7d77f8ceb2d95dbe8a 
>   src/tests/uri_tests.cpp 1d33ab110bed7c0b7ecfa5d608965da5a1729562 
>   src/uri/utils.cpp 3940dc041c1783eec9e7c950402fd7c42e620d8e 
> 
> 
> Diff: https://reviews.apache.org/r/71441/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71441: Fixed Docker registry malformed URI contruction.

[James Peach]

> On Sept. 16, 2019, 5:24 a.m., Qian Zhang wrote:
> > In the description of this patch, I see you mentioned we only have this 
> > issue with a non-default Docker registry, but that seems not what I found. 
> > I started Mesos agent without setting the flag `--docker_registry` (so it 
> > just took the default value: `https://registry-1.docker.io`) and then 
> > launched a container with an image named `zhq527725/whiteout:latest`, and 
> > then I see:
> > > Pulling image 'zhq527725/whiteout:latest' from 
> > > 'docker-manifest://registry-1.docker.io:443zhq527725/whiteout?latest#https'
> > >  to '/opt/qzhang/mesos/store/docker/staging/pKccqL'
> > 
> > As you see, the separator between the hostname and the path was missed as 
> > well, and I found the container can be launched successfully. So what is 
> > actually the issue? Just the malformed registry URLs in the above message? 
> > If I read MESOS-9963 correctly, the issue should be launching container 
> > fails, right?
> 
> James Peach wrote:
> In my testing, the malformed URL is used to pull the images, and that 
> fails. Is there a code path I'm missing?
> 
> Qian Zhang wrote:
> Can you please let me know the detailed error message in your testing?
> 
> IIUC, the issue (missing separator) is in the method `ostream& 
> operator<<(ostream& stream, const URI& uri)`, and that method is used to 
> print the message `Pulling image ...` but not used when actually fetching 
> image, right?
> 
> James Peach wrote:
> The curl fetcher uses it via 
> [stringify](https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L129).
> 
> Qian Zhang wrote:
> I think you are talking about [this 
> code](https://github.com/apache/mesos/blob/1.9.0/src/uri/fetchers/docker.cpp#L235),
>  right? However the URI used in that code is not the malformed URI that you 
> mentioned in this patch, it is actually re-constructed from the malformed URI 
> (see `DockerFetcherPluginProcess::getManifestUri` and 
> `DockerFetcherPluginProcess::getBlobUri`), so even we see the malformed URL 
> in the message like this 
> `"docker-manifest://registry-1.docker.io:443zhq527725/whiteout?latest#https"`,
>  the URI used by curl is still correct, like: 
> `"https://registry-1.docker.io:443/v2/zhq527725/whiteout/manifests/latest"`.

Ah I see. The Docker puller makes sure to place a leading '/' in the path. I 
asked the relevant team to re-test and they confirmed that this is purely a 
cosmetic issue.

I still think that this change is correct, but I'll update the commit message :)


- James


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


On Sept. 6, 2019, 6:20 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71441/
> ---
> 
> (Updated Sept. 6, 2019, 6:20 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-9963
> https://issues.apache.org/jira/browse/MESOS-9963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a non-default Docker registry is used for container image, the Docker
> puller would omit the separator between the hostname and the path when
> constructing the final URI.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_spec_tests.cpp 
> 4be0716895132e0406299f7d77f8ceb2d95dbe8a 
>   src/tests/uri_tests.cpp 1d33ab110bed7c0b7ecfa5d608965da5a1729562 
>   src/uri/utils.cpp 3940dc041c1783eec9e7c950402fd7c42e620d8e 
> 
> 
> Diff: https://reviews.apache.org/r/71441/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71487: Added method to notify allocator that offered transitioned to allocated.

[Meng Zhu]

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




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


`launchResources`? 
Matches better with `remainingResources`.



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


why the newline? let's minimize unrelated changes.


- Meng Zhu


On Sept. 17, 2019, 6:29 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71487/
> ---
> 
> (Updated Sept. 17, 2019, 6:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9949
> https://issues.apache.org/jira/browse/MESOS-9949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a prerequisite for tracking quota consumption in allocator.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 2bab53ab5fb25931a724c20a039e1301983ba574 
>   src/master/allocator/mesos/allocator.hpp 
> 6921581745bf876ee42bcfb62b59245f23fcbf47 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5ea37912dadded47eaa2d354889c95533b191c59 
>   src/master/allocator/mesos/hierarchical.cpp 
> 256fdce61ccfb768605cac1579c9a6632cd26fec 
>   src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
>   src/tests/allocator.hpp 01e6d2ce3d0655ad408f605c7be84cd7c0f0d479 
> 
> 
> Diff: https://reviews.apache.org/r/71487/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 71488: Track per-role allocated resources in the roles tree.

[Meng Zhu]

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




src/master/allocator/mesos/hierarchical.hpp
Lines 134 (patched)


why not resource quantities? what matter data do we need?


- Meng Zhu


On Sept. 17, 2019, 9:48 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71488/
> ---
> 
> (Updated Sept. 17, 2019, 9:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9949
> https://issues.apache.org/jira/browse/MESOS-9949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track per-role allocated resources in the roles tree.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5ea37912dadded47eaa2d354889c95533b191c59 
>   src/master/allocator/mesos/hierarchical.cpp 
> 256fdce61ccfb768605cac1579c9a6632cd26fec 
> 
> 
> Diff: https://reviews.apache.org/r/71488/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 71498: Introduced a role tree helper to modify a role and all its ancestors.

[Meng Zhu]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp
Lines 261 (patched)


why not take in a `Role*`? It's one less lookup and clearly shifts the 
burden of `CHECK` to the caller.
I think the value of this helper is to eliminate the pointer invovled loop. 
Taking the string is not necessary.


- Meng Zhu


On Sept. 17, 2019, 6:25 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71498/
> ---
> 
> (Updated Sept. 17, 2019, 6:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9949
> https://issues.apache.org/jira/browse/MESOS-9949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a role tree helper to modify a role and all its ancestors.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5ea37912dadded47eaa2d354889c95533b191c59 
>   src/master/allocator/mesos/hierarchical.cpp 
> 256fdce61ccfb768605cac1579c9a6632cd26fec 
> 
> 
> Diff: https://reviews.apache.org/r/71498/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 71486: Added `allocated` flag to the `Allocator::recoverResources()` method.

[Meng Zhu]

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



Looking at the next patch, it looks like we use `recoverResources(true)` to 
untrack the allocated resources. And use a separate function to 
`trackAllocated`.

I think this asymmetry will definitely lead to some frowns on caller/reader. I 
would suggest let's do an explicitly untrack independent of the recover call. 
Reasons being:

- The symmetry would be easier to read and less error-prone (forgetting to call 
untrack)
- It clearly indicates that `allocated` is a `tracked` value that passively 
follows the master action
- It avoid any changes to the current recoverResources

One minor concern is that people might forget to call track/Untrack altogether. 
If we can have a launch helper or something in the master, that would be great. 
But don't worry too much about it if it is non-trivial work.


include/mesos/allocator/allocator.hpp
Lines 393 (patched)


I know it is for backward compatibility. But it is strange to have a 
default value here in the first glance. It also forces callers to be explicit 
about setting the field correctly.

Also, I think it is generally not a good idea to add default arguments to 
the interface.

This comment might not be relevant, see my top-level comment.


- Meng Zhu


On Sept. 17, 2019, 6:30 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71486/
> ---
> 
> (Updated Sept. 17, 2019, 6:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9949
> https://issues.apache.org/jira/browse/MESOS-9949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends the signature of `recoverResources()` with a flag
> indicating wheteher the resources being recovered were actaully
> allocated to a framework, or only offered.
> 
> This is a prerequisite for tracking quota consumption in the allocator.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 2bab53ab5fb25931a724c20a039e1301983ba574 
>   src/master/allocator/mesos/allocator.hpp 
> 6921581745bf876ee42bcfb62b59245f23fcbf47 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5ea37912dadded47eaa2d354889c95533b191c59 
>   src/master/allocator/mesos/hierarchical.cpp 
> 256fdce61ccfb768605cac1579c9a6632cd26fec 
>   src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
>   src/tests/allocator.hpp 01e6d2ce3d0655ad408f605c7be84cd7c0f0d479 
>   src/tests/master_allocator_tests.cpp 
> e9607577c57c4ae36133a649667b5a416ba9dece 
>   src/tests/slave_recovery_tests.cpp d99752ab082f1aca9fb77659378d0bca5a0598eb 
> 
> 
> Diff: https://reviews.apache.org/r/71486/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 71498: Introduced a role tree helper to modify a role and all its ancestors.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71489, 71486, 71487, 71488, 71490, 71491, 71498]

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

- Mesos Reviewbot


On Sept. 17, 2019, 1:25 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71498/
> ---
> 
> (Updated Sept. 17, 2019, 1:25 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9949
> https://issues.apache.org/jira/browse/MESOS-9949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a role tree helper to modify a role and all its ancestors.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5ea37912dadded47eaa2d354889c95533b191c59 
>   src/master/allocator/mesos/hierarchical.cpp 
> 256fdce61ccfb768605cac1579c9a6632cd26fec 
> 
> 
> Diff: https://reviews.apache.org/r/71498/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

[Vinod Kone]

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



drive by review.


3rdparty/libprocess/src/openssl.cpp
Lines 118 (patched)


can you use the `alias` argument in `add()` for these?



docs/ssl.md
Lines 100 (patched)


These deprecations need to be documented in CHANGELOG and upgrades.md.


- Vinod Kone


On Sept. 17, 2019, 12:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
> 
> (Updated Sept. 17, 2019, 12:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
> `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.
> 
> The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
> `LIBPROCESS_SSL_VERIFY_SERVER_CERT`.
> 
> The new names better describe the actual effect of both flags, and
> make upgrades easier by allowing operators to only enable verification
> on agents that are new enough to contain the updated hostname
> validation code paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 1a0e3820cc8cd1459625f46a54b194133500f11e 
>   3rdparty/libprocess/src/openssl.hpp 
> 271cc95238d287c06df36478554502e8b7205b09 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 9d5ab679165a709f7c3740020961ec89a7db4f54 
>   docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
> 
> 
> Diff: https://reviews.apache.org/r/71497/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65042: Adjusted CSI example framework for recent changes.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [64932, 65042]

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

- Mesos Reviewbot


On Sept. 17, 2019, 1 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65042/
> ---
> 
> (Updated Sept. 17, 2019, 1 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adjusts the CSI example framework for recent cleanups to
> example frameworks. It now uses generic flag classes which can e.g.,
> parse from both the environment and the command line.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_user_framework.cpp 
> 1bf5d14859be5ec4e7ff351ab34a9b5752080146 
> 
> 
> Diff: https://reviews.apache.org/r/65042/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71488: Track per-role allocated resources in the roles tree.

[Andrei Sekretenko]

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

(Updated Sept. 17, 2019, 4:48 p.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Changes
---

Allocated resources should be untracked regardless of whether the framework has 
been untracked.


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


Repository: mesos


Description
---

Track per-role allocated resources in the roles tree.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 71430: Included new Python CLI in distribution tarball.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Sept. 4, 2019, 7:48 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71430/
> ---
> 
> (Updated Sept. 4, 2019, 7:48 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Till Toenshoff.
> 
> 
> Bugs: MESOS-9958
> https://issues.apache.org/jira/browse/MESOS-9958
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Included new Python CLI in distribution tarball.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5daee632d52e882acc15b90f655a53dab23eaaf6 
> 
> 
> Diff: https://reviews.apache.org/r/71430/diff/1/
> 
> 
> Testing
> ---
> 
> * confirmed that the new CLI source files are contained in the distribution 
> tarball
> * one can currently still not run `make distcheck` with `--enable-new-cli` 
> since the Mesos lib egg build process puts output files into the source dir
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71496: Removed an outdated reference to the 'libprocess' hostname validation.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Sept. 17, 2019, 12:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71496/
> ---
> 
> (Updated Sept. 17, 2019, 12:35 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a reference to the 'libprocess' hostname validation scheme,
> which was removed to 'legacy' during development.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
> 
> 
> Diff: https://reviews.apache.org/r/71496/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71496, 71497]

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

- Mesos Reviewbot


On Sept. 17, 2019, 12:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
> 
> (Updated Sept. 17, 2019, 12:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
> `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.
> 
> The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
> `LIBPROCESS_SSL_VERIFY_SERVER_CERT`.
> 
> The new names better describe the actual effect of both flags, and
> make upgrades easier by allowing operators to only enable verification
> on agents that are new enough to contain the updated hostname
> validation code paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 1a0e3820cc8cd1459625f46a54b194133500f11e 
>   3rdparty/libprocess/src/openssl.hpp 
> 271cc95238d287c06df36478554502e8b7205b09 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 9d5ab679165a709f7c3740020961ec89a7db4f54 
>   docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
> 
> 
> Diff: https://reviews.apache.org/r/71497/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71489: Replaced removal+adding of quota metrics with updating them.

[Andrei Sekretenko]

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

(Updated Sept. 17, 2019, 1:35 p.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch factor out the common code for updating quota guarantee
and quota limit metrics, and replaces pairs of
`process::metrics::remove()`/`process::metrics::add()` called for the
same metric with calling `PushGauge::operator = ()`. This is
a perequisite for adding PushGauge-based quota consumption metrics.


Diffs
-

  src/master/allocator/mesos/metrics.hpp 
adb9515c82029f32e57e73cddd441c1bceec1aee 
  src/master/allocator/mesos/metrics.cpp 
ba95dc4415e3f85522f5d1291b6e9c171b900482 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71486: Added `allocated` flag to the `Allocator::recoverResources()` method.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch extends the signature of `recoverResources()` with a flag
indicating wheteher the resources being recovered were actaully
allocated to a framework, or only offered.

This is a prerequisite for tracking quota consumption in the allocator.


Diffs
-

  include/mesos/allocator/allocator.hpp 
2bab53ab5fb25931a724c20a039e1301983ba574 
  src/master/allocator/mesos/allocator.hpp 
6921581745bf876ee42bcfb62b59245f23fcbf47 
  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 
  src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
  src/tests/allocator.hpp 01e6d2ce3d0655ad408f605c7be84cd7c0f0d479 
  src/tests/master_allocator_tests.cpp e9607577c57c4ae36133a649667b5a416ba9dece 
  src/tests/slave_recovery_tests.cpp d99752ab082f1aca9fb77659378d0bca5a0598eb 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71487: Added method to notify allocator that offered transitioned to allocated.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This is a prerequisite for tracking quota consumption in allocator.


Diffs
-

  include/mesos/allocator/allocator.hpp 
2bab53ab5fb25931a724c20a039e1301983ba574 
  src/master/allocator/mesos/allocator.hpp 
6921581745bf876ee42bcfb62b59245f23fcbf47 
  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 
  src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
  src/tests/allocator.hpp 01e6d2ce3d0655ad408f605c7be84cd7c0f0d479 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71488: Track per-role allocated resources in the roles tree.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

Track per-role allocated resources in the roles tree.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71489: Replaced removal+adding of quota metrics with updating them.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch factor out the common code for updating quota guarantee
and quota limit metrics, and replaces pairs of
`process::metrics::remove()`/`process::metrics::add()` called for the
same metric with calling `PushGauge::operator = ()`. This is
a perequisite for adding PushGauge-based quota consumption metrics.


Diffs
-

  src/master/allocator/mesos/metrics.hpp 
adb9515c82029f32e57e73cddd441c1bceec1aee 
  src/master/allocator/mesos/metrics.cpp 
ba95dc4415e3f85522f5d1291b6e9c171b900482 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71490: Added quota consumption metrics to the hierarchial allocator.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

Added quota consumption metrics to the hierarchial allocator.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 
  src/master/allocator/mesos/metrics.hpp 
adb9515c82029f32e57e73cddd441c1bceec1aee 
  src/master/allocator/mesos/metrics.cpp 
ba95dc4415e3f85522f5d1291b6e9c171b900482 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71491: Added tests of metrics for tracking quota consumption.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


Bugs: MESOS-9123 and MESOS-9949
https://issues.apache.org/jira/browse/MESOS-9123
https://issues.apache.org/jira/browse/MESOS-9949


Repository: mesos


Description
---

Added tests of metrics for tracking quota consumption.


Diffs
-

  src/Makefile.am 79bf77d9b7401016af632232cb62f3929cfeee5b 
  src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
  src/tests/consumption_metrics_tests.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 71498: Introduced a role tree helper to modify a role and all its ancestors.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

Introduced a role tree helper to modify a role and all its ancestors.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 


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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 65042: Adjusted CSI example framework for recent changes.

[Benjamin Bannier]

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

(Updated Sept. 17, 2019, 3 p.m.)


Review request for mesos, Gastón Kleiman and Till Toenshoff.


Changes
---

Rebase


Repository: mesos


Description
---

This patch adjusts the CSI example framework for recent cleanups to
example frameworks. It now uses generic flag classes which can e.g.,
parse from both the environment and the command line.


Diffs (updated)
-

  src/examples/test_csi_user_framework.cpp 
1bf5d14859be5ec4e7ff351ab34a9b5752080146 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 71497: Introduced new names for SSL-related libprocess flags.

[Benno Evers]

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

Review request for mesos, Greg Mann and Till Toenshoff.


Repository: mesos


Description
---

The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
`LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.

The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
`LIBPROCESS_SSL_VERIFY_SERVER_CERT`.

The new names better describe the actual effect of both flags, and
make upgrades easier by allowing operators to only enable verification
on agents that are new enough to contain the updated hostname
validation code paths.


Diffs
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
1a0e3820cc8cd1459625f46a54b194133500f11e 
  3rdparty/libprocess/src/openssl.hpp 271cc95238d287c06df36478554502e8b7205b09 
  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
9d5ab679165a709f7c3740020961ec89a7db4f54 
  docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 


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


Testing
---


Thanks,

Benno Evers

Review Request 71496: Removed an outdated reference to the 'libprocess' hostname validation.

[Benno Evers]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Removed a reference to the 'libprocess' hostname validation scheme,
which was removed to 'legacy' during development.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 71494: Removed documentation on cquery.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71494]

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

- Mesos Reviewbot


On Sept. 17, 2019, 7:44 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71494/
> ---
> 
> (Updated Sept. 17, 2019, 7:44 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benno Evers, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cquery is not developed anymore and being replaced by larger LSP
> implementation such as clangd. This patch completely removes any
> cquery-related documentation instead of updating it to use e.g., clangd.
> This is because any documentation related to setting up a LSP pipeline
> would be almost exclusively generic to a CMake C++ project and not
> specific to Mesos.
> 
> 
> Diffs
> -
> 
>   docs/cquery.md b893bc522561306a2cdf15e1c1876108ec8d329c 
>   docs/developer-guide.md 3906275e18801853bd21da18073c9f0e44aba2cd 
> 
> 
> Diff: https://reviews.apache.org/r/71494/diff/1/
> 
> 
> Testing
> ---
> 
> Test-rendered the documentation
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71441: Fixed Docker registry malformed URI contruction.

[Qian Zhang]

> On Sept. 16, 2019, 1:24 p.m., Qian Zhang wrote:
> > In the description of this patch, I see you mentioned we only have this 
> > issue with a non-default Docker registry, but that seems not what I found. 
> > I started Mesos agent without setting the flag `--docker_registry` (so it 
> > just took the default value: `https://registry-1.docker.io`) and then 
> > launched a container with an image named `zhq527725/whiteout:latest`, and 
> > then I see:
> > > Pulling image 'zhq527725/whiteout:latest' from 
> > > 'docker-manifest://registry-1.docker.io:443zhq527725/whiteout?latest#https'
> > >  to '/opt/qzhang/mesos/store/docker/staging/pKccqL'
> > 
> > As you see, the separator between the hostname and the path was missed as 
> > well, and I found the container can be launched successfully. So what is 
> > actually the issue? Just the malformed registry URLs in the above message? 
> > If I read MESOS-9963 correctly, the issue should be launching container 
> > fails, right?
> 
> James Peach wrote:
> In my testing, the malformed URL is used to pull the images, and that 
> fails. Is there a code path I'm missing?
> 
> Qian Zhang wrote:
> Can you please let me know the detailed error message in your testing?
> 
> IIUC, the issue (missing separator) is in the method `ostream& 
> operator<<(ostream& stream, const URI& uri)`, and that method is used to 
> print the message `Pulling image ...` but not used when actually fetching 
> image, right?
> 
> James Peach wrote:
> The curl fetcher uses it via 
> [stringify](https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L129).

I think you are talking about [this 
code](https://github.com/apache/mesos/blob/1.9.0/src/uri/fetchers/docker.cpp#L235),
 right? However the URI used in that code is not the malformed URI that you 
mentioned in this patch, it is actually re-constructed from the malformed URI 
(see `DockerFetcherPluginProcess::getManifestUri` and 
`DockerFetcherPluginProcess::getBlobUri`), so even we see the malformed URL in 
the message like this 
`"docker-manifest://registry-1.docker.io:443zhq527725/whiteout?latest#https"`, 
the URI used by curl is still correct, like: 
`"https://registry-1.docker.io:443/v2/zhq527725/whiteout/manifests/latest"`.


- Qian


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


On Sept. 6, 2019, 2:20 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71441/
> ---
> 
> (Updated Sept. 6, 2019, 2:20 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-9963
> https://issues.apache.org/jira/browse/MESOS-9963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a non-default Docker registry is used for container image, the Docker
> puller would omit the separator between the hostname and the path when
> constructing the final URI.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_spec_tests.cpp 
> 4be0716895132e0406299f7d77f8ceb2d95dbe8a 
>   src/tests/uri_tests.cpp 1d33ab110bed7c0b7ecfa5d608965da5a1729562 
>   src/uri/utils.cpp 3940dc041c1783eec9e7c950402fd7c42e620d8e 
> 
> 
> Diff: https://reviews.apache.org/r/71441/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 71494: Removed documentation on cquery.

[Benjamin Bannier]

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

Review request for mesos, Andrew Schwartzmeyer, Benno Evers, and Till Toenshoff.


Repository: mesos


Description
---

Cquery is not developed anymore and being replaced by larger LSP
implementation such as clangd. This patch completely removes any
cquery-related documentation instead of updating it to use e.g., clangd.
This is because any documentation related to setting up a LSP pipeline
would be almost exclusively generic to a CMake C++ project and not
specific to Mesos.


Diffs
-

  docs/cquery.md b893bc522561306a2cdf15e1c1876108ec8d329c 
  docs/developer-guide.md 3906275e18801853bd21da18073c9f0e44aba2cd 


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


Testing
---

Test-rendered the documentation


Thanks,

Benjamin Bannier