Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Jay Guo]

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




src/master/master.cpp (lines 3944 - 3950)


I think we explicitly disallow empyt role field? 
https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
Also, should the case `framework->info.has_role() == false && 
framework->info.role() != "*"` be invalid and caught upon subscription?


- Jay Guo


On Jan. 12, 2017, 10:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Jan. 12, 2017, 10:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Jay Guo]

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



When a resource with `*` is offered to a multi-role framework, how does the 
framework decide which role to reserve the resource for?

- Jay Guo


On Jan. 12, 2017, 10:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Jan. 12, 2017, 10:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[Jiang Yan Xu]

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




3rdparty/stout/include/stout/os/posix/chown.hpp (lines 36 - 42)


We are handling the situation where we can't `stat` the root. Is it a 
file/dir permission concern? 

What if we can't `stat` the subdirs and files? 

I seems to me that for the two things we are doing:

1) Only descend if the root is a directory and `recursive` is true: 

If we just put a 
```
if (!recursive && node->fts_level != FTS_ROOTLEVEL) {
  break;
}
```

inside the while loop so we only need to call `lchown` from one place.

2) Dealing with issues with `stat` (and other errros)

We need to handle then inside the loop too. Even though `chown` requires a 
"privledged" user (unless if you chown to yourself I think). I guess we 
shouldn't assume this while traversing the tree and should let the system call 
fail themselves at appropriate places.



3rdparty/stout/include/stout/os/posix/chown.hpp (line 43)


s/char */char*/

s/paths_/paths/ since there's no `paths`?



3rdparty/stout/include/stout/os/posix/chown.hpp (lines 52 - 53)


`node` should align with `FTSENT` and we should put each expression on a 
new line.

However the following calls `node = fts_read(tree)` from just one place 
which I think is nicer?

```
FTSENT* node;
while ((node = ::fts_read(tree)) != nullptr) {

}
```



3rdparty/stout/include/stout/os/posix/chown.hpp (lines 54 - 56)


Will the inability to `stat` the file result in `FTS_NS`? Do we not need to 
handle other fts errors?

If we don't, our `if` condition is going to silently ignore these files and 
report success.



3rdparty/stout/tests/os_tests.cpp (line 728)


Unfortunately it's a bit tricky to test the permission stuff I guess.



3rdparty/stout/tests/os_tests.cpp (line 746)


s/link/symlink/



3rdparty/stout/tests/os_tests.cpp (line 751)


s/tree/subtree/?



3rdparty/stout/tests/os_tests.cpp (line 752)


Comment that `9` is just an arbitrary uid that we picked?


- Jiang Yan Xu


On Jan. 11, 2017, 4:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> ---
> 
> (Updated Jan. 11, 2017, 4:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reimplement os::chown() to use fts(3) rather than sometimes spawning
> chown(1). This removes the use of the shell and the corresponding
> need to sanitize path arguments.  It also enables us to provide
> consistent handling of symbolic links for the recursive and
> non-recursive cases. We ensure that symlinks are never followed
> and that we always change the ownership of the link itself, not
> its referent.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/chown.hpp 
> c82e2e574019c5ee5f17ea105a6d225006388a45 
>   3rdparty/stout/include/stout/os/posix/stat.hpp 
> 1ab20e75fc18b336162b62e2f4f23b68f6685183 
>   3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 
> 
> Diff: https://reviews.apache.org/r/55242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55449: Validate executor IDs in master validation.

[Jiang Yan Xu]

> On Jan. 12, 2017, 4:31 p.m., Joseph Wu wrote:
> > src/tests/master_validation_tests.cpp, line 1751
> > 
> >
> > I had the impression that spaces in IDs were also bad...
> > 
> > I'm hoping we don't use the sandbox path in a shell anywhere anymore.

I agree but it's not one of the current rules. I am actually hoping to add a 
few more other rules (e.g., max length) but plan to do it separately so folks 
can discuss them. Here's it's checking against the current rules (which we 
unfortunately don't validate at all).


- Jiang Yan


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


On Jan. 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55449/
> ---
> 
> (Updated Jan. 12, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, James Peach and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validate executor IDs in master validation.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55449/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 55490: Used the `mesos/mesos-tidy` image from DockerHub.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 

Diff: https://reviews.apache.org/r/55490/diff/


Testing
---


Thanks,

Michael Park

Review Request 55488: Slimmed down the size of the docker image from 2GB to roughly 650MB.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 

Diff: https://reviews.apache.org/r/55488/diff/


Testing
---


Thanks,

Michael Park

Review Request 55487: Renamed executable files from '_' to '-' in the `support` directory.

[Michael Park]

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

Review request for mesos, Benjamin Bannier and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/docker_build.sh  
  support/hooks/post-rewrite e3747a320fef0b71c06bcf0f2c5532958c416646 
  support/hooks/pre-commit 10838a4c99db2a8318d64f95d90d2c2c90150453 
  support/mesos_split.py  
  support/timed_tests.sh  
  support/verify_reviews.py  

Diff: https://reviews.apache.org/r/55487/diff/


Testing
---


Thanks,

Michael Park

Review Request 55489: Used CMake to generate the compilation database instead.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 
  support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
  support/mesos-tidy/entrypoint.sh 72872375f3e5ad19bc75949f9e3db14d6068f9b6 

Diff: https://reviews.apache.org/r/55489/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 55489: Used CMake to generate the compilation database instead.

[Michael Park]

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

(Updated Jan. 13, 2017, 1:59 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

Used CMake to generate the compilation database instead.


Diffs (updated)
-

  support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 
  support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
  support/mesos-tidy/entrypoint.sh 72872375f3e5ad19bc75949f9e3db14d6068f9b6 

Diff: https://reviews.apache.org/r/55489/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 55490: Used the `mesos/mesos-tidy` image from DockerHub.

[Michael Park]

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

(Updated Jan. 13, 2017, 2 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

Used the `mesos/mesos-tidy` image from DockerHub.


Diffs (updated)
-

  support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 

Diff: https://reviews.apache.org/r/55490/diff/


Testing
---


Thanks,

Michael Park

Review Request 55491: Added a `jenkins/tidybot.sh`.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/jenkins/tidybot.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/55491/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 55464: Made the Agent API able to handle containers nested at arbitrary levels.

[Gastón Kleiman]

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

(Updated Jan. 13, 2017, 10:08 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
Alexander Rojas, and haosdent huang.


Changes
---

Added a test.


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


Repository: mesos


Description
---

Made the Agent API able to handle containers nested at arbitrary levels.


Diffs (updated)
-

  src/slave/http.cpp 24fc23b229c624835a24cdda9587c99c6ac9c3bb 
  src/slave/slave.cpp 11e8833fd5998abb71a7bb08e5dec451d894aba9 
  src/tests/api_tests.cpp ea62226fbc5dd0bcc5ea60d1bbcd3748f9367467 

Diff: https://reviews.apache.org/r/55464/diff/


Testing (updated)
---

`GTEST_FILTER="" make -j 8 check` in macOS.
`make check` in Linux.

I also did some manual testing using a proof of concept  that makes the 
`DefaultExecutor` leverage this change to perform CMD health checks.


Thanks,

Gastón Kleiman

Re: Review Request 55464: Made the Agent API able to handle containers nested at arbitrary levels.

[Gastón Kleiman]

> On Jan. 12, 2017, 3:50 p.m., Alexander Rojas wrote:
> > src/tests/api_tests.cpp, lines 3619-3659
> > 
> >
> > Why not replacing this test with one that shows the opposite?

I added a test that launches parent and child containers nested under an 
executor, kills them and waits for them to be completed.


- Gastón


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


On Jan. 13, 2017, 10:08 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55464/
> ---
> 
> (Updated Jan. 13, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Alexander Rojas, and haosdent huang.
> 
> 
> Bugs: MESOS-6864
> https://issues.apache.org/jira/browse/MESOS-6864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the Agent API able to handle containers nested at arbitrary levels.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 24fc23b229c624835a24cdda9587c99c6ac9c3bb 
>   src/slave/slave.cpp 11e8833fd5998abb71a7bb08e5dec451d894aba9 
>   src/tests/api_tests.cpp ea62226fbc5dd0bcc5ea60d1bbcd3748f9367467 
> 
> Diff: https://reviews.apache.org/r/55464/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="" make -j 8 check` in macOS.
> `make check` in Linux.
> 
> I also did some manual testing using a proof of concept  that makes the 
> `DefaultExecutor` leverage this change to perform CMD health checks.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55491: Added a `jenkins/tidybot.sh`.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [55487, 55488, 55489, 55490, 55491]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh 2>&1 | tee 
build_55491"]

Error:
bash: ./support/docker_build.sh: No such file or directory

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16710/console

- Mesos ReviewBot


On Jan. 13, 2017, 10:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55491/
> ---
> 
> (Updated Jan. 13, 2017, 10:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/jenkins/tidybot.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55491/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 55496: Fixed parsing of proxy CONNECT responses.

[Jan Schlicht]

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

Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

When behind a proxy, access to websites using SSL can be done by using
HTTP CONNECT tunneling. This is, for example, used when we pull images
from a Docker repository. The HTTP response of such a CONNECT request
surfaced a problem with our HTTP parser, because it doesn't have neither
any headers nor a message body. The parser expected a 'Content-Length'
header to be set in every HTTP response. We fix this by assuming that
a HTTP response doesn't have a message body when the 'Content-Length'
header isn't set.


Diffs
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
87563f4fcdfaa2a33c4482533ddff24e062e603a 

Diff: https://reviews.apache.org/r/55496/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 55488: Slimmed down the size of the docker image from 2GB to roughly 650MB.

[Benjamin Bannier]

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


Fix it, then Ship it!





support/mesos-tidy/Dockerfile 


Agree that this one is better in its own section.



support/mesos-tidy/Dockerfile (lines 68 - 71)


These packages are from the Mesos getting started guide, and I am not sure 
we would like to deviate here. It does save about 400MB in this layer (probably 
since we do not install the java packages), but we'll likely be perpetually out 
of sync as the Mesos cmake setup approaches the autotool setup's capabilities.



support/mesos-tidy/Dockerfile (line 76)


Please wrap this

RUN apt-get install -qy \
  jq \
  parallel
  
Not yours, but it probably makes sense to terminate the command with an 
`apt-get clean` just for consistency (this safes almost no space for that 
layer).


- Benjamin Bannier


On Jan. 13, 2017, 10:54 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55488/
> ---
> 
> (Updated Jan. 13, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
> 
> Diff: https://reviews.apache.org/r/55488/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55489: Used CMake to generate the compilation database instead.

[Benjamin Bannier]

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




support/mesos-tidy.sh (line 19)


Should we also set `pipefail`?



support/mesos-tidy.sh 


It seems something like this would still be useful for the cmake build so 
callers could pass extra configuration flags (e.g., of the form 
`-DENABLE_DEBUG=ON`).



support/mesos-tidy/entrypoint.sh (line 19)


Should we also set `pipefail`?



support/mesos-tidy/entrypoint.sh (line 27)


Left over debug tooling?



support/mesos-tidy/entrypoint.sh (line 33)


I don't think the folks interested in cmake would ever see this comment 
here. We should create tickets and link them here.



support/mesos-tidy/entrypoint.sh (line 47)


Dito.


- Benjamin Bannier


On Jan. 13, 2017, 10:59 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55489/
> ---
> 
> (Updated Jan. 13, 2017, 10:59 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used CMake to generate the compilation database instead.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 
>   support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
>   support/mesos-tidy/entrypoint.sh 72872375f3e5ad19bc75949f9e3db14d6068f9b6 
> 
> Diff: https://reviews.apache.org/r/55489/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55453: Updated comments in `HealthCheck` protobuf for clarity.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Jan. 12, 2017, 1:18 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> ---
> 
> (Updated Jan. 12, 2017, 1:18 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab68ff85c4af5d254779b30a7f27eda9fcb790eb 
>   include/mesos/v1/mesos.proto caefa239be6ead10b9a5fc91ba120ea9c8775313 
> 
> Diff: https://reviews.apache.org/r/55453/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55491: Added a `jenkins/tidybot.sh`.

[Benjamin Bannier]

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



Is the intention here to add more stuff later, or move configuration into 
version control? Right now this seems as if Jenkins could just be configured to 
point to the script directly; we'd then be able to tweak the scripts.

- Benjamin Bannier


On Jan. 13, 2017, 11:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55491/
> ---
> 
> (Updated Jan. 13, 2017, 11:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/jenkins/tidybot.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55491/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55454: Ensured zero health check timeout means infinite timeout.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Jan. 12, 2017, 1:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55454/
> ---
> 
> (Updated Jan. 12, 2017, 1:19 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6908
> https://issues.apache.org/jira/browse/MESOS-6908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, zero health check timeout was interpreted
> literally, which is not very helpful since a health check did not
> even get a chance to finish. This patch fixes this behaviour by
> interpreting zero as `Duration::max()` effectively rendering the
> timeout infinite.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab68ff85c4af5d254779b30a7f27eda9fcb790eb 
>   include/mesos/v1/mesos.proto caefa239be6ead10b9a5fc91ba120ea9c8775313 
>   src/health-check/health_checker.cpp 
> a8424b75927d15dc1b897faf0e47cf075c70ff26 
> 
> Diff: https://reviews.apache.org/r/55454/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55456: Fixed include order in "launcher/executor.cpp".

[Gastón Kleiman]

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




src/launcher/executor.cpp (line 79)


According to our style guide, this should be the first include =)


- Gastón Kleiman


On Jan. 12, 2017, 1:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55456/
> ---
> 
> (Updated Jan. 12, 2017, 1:19 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa 
> 
> Diff: https://reviews.apache.org/r/55456/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55464: Made the Agent API able to handle containers nested at arbitrary levels.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55463, 55464]

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 Jan. 13, 2017, 10:08 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55464/
> ---
> 
> (Updated Jan. 13, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Alexander Rojas, and haosdent huang.
> 
> 
> Bugs: MESOS-6864
> https://issues.apache.org/jira/browse/MESOS-6864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the Agent API able to handle containers nested at arbitrary levels.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 24fc23b229c624835a24cdda9587c99c6ac9c3bb 
>   src/slave/slave.cpp 11e8833fd5998abb71a7bb08e5dec451d894aba9 
>   src/tests/api_tests.cpp ea62226fbc5dd0bcc5ea60d1bbcd3748f9367467 
> 
> Diff: https://reviews.apache.org/r/55464/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="" make -j 8 check` in macOS.
> `make check` in Linux.
> 
> I also did some manual testing using a proof of concept  that makes the 
> `DefaultExecutor` leverage this change to perform CMD health checks.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.

[Jan Schlicht]

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



Don't merge yet, this seems to break chunked transfers.

- Jan Schlicht


On Jan. 13, 2017, 12:28 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55496/
> ---
> 
> (Updated Jan. 13, 2017, 12:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6010
> https://issues.apache.org/jira/browse/MESOS-6010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When behind a proxy, access to websites using SSL can be done by using
> HTTP CONNECT tunneling. This is, for example, used when we pull images
> from a Docker repository. The HTTP response of such a CONNECT request
> surfaced a problem with our HTTP parser, because it doesn't have neither
> any headers nor a message body. The parser expected a 'Content-Length'
> header to be set in every HTTP response. We fix this by assuming that
> a HTTP response doesn't have a message body when the 'Content-Length'
> header isn't set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> 87563f4fcdfaa2a33c4482533ddff24e062e603a 
> 
> Diff: https://reviews.apache.org/r/55496/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.

[Jan Schlicht]

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

(Updated Jan. 13, 2017, 2:48 p.m.)


Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.


Changes
---

Fixed problems with chunked transfer encodings.


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


Repository: mesos


Description
---

When behind a proxy, access to websites using SSL can be done by using
HTTP CONNECT tunneling. This is, for example, used when we pull images
from a Docker repository. The HTTP response of such a CONNECT request
surfaced a problem with our HTTP parser, because it doesn't have neither
any headers nor a message body. The parser expected a 'Content-Length'
header to be set in every HTTP response. We fix this by assuming that
a HTTP response doesn't have a message body when the 'Content-Length'
header isn't set.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
87563f4fcdfaa2a33c4482533ddff24e062e603a 

Diff: https://reviews.apache.org/r/55496/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 55464: Made the Agent API able to handle containers nested at arbitrary levels.

[Gastón Kleiman]

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

(Updated Jan. 13, 2017, 1:52 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
Alexander Rojas, and haosdent huang.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Made the Agent API able to handle containers nested at arbitrary levels.


Diffs (updated)
-

  src/slave/http.cpp 24fc23b229c624835a24cdda9587c99c6ac9c3bb 
  src/slave/slave.cpp 11e8833fd5998abb71a7bb08e5dec451d894aba9 
  src/tests/api_tests.cpp ea62226fbc5dd0bcc5ea60d1bbcd3748f9367467 

Diff: https://reviews.apache.org/r/55464/diff/


Testing
---

`GTEST_FILTER="" make -j 8 check` in macOS.
`make check` in Linux.

I also did some manual testing using a proof of concept  that makes the 
`DefaultExecutor` leverage this change to perform CMD health checks.


Thanks,

Gastón Kleiman

Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.

[Benjamin Bannier]

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




3rdparty/libprocess/src/decoder.hpp (line 416)


If you made `encoding` an `Option` and dropped the `getOrElse` 
unwrapping, you could directly compare it against `Some("chunked")` below.


- Benjamin Bannier


On Jan. 13, 2017, 2:48 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55496/
> ---
> 
> (Updated Jan. 13, 2017, 2:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6010
> https://issues.apache.org/jira/browse/MESOS-6010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When behind a proxy, access to websites using SSL can be done by using
> HTTP CONNECT tunneling. This is, for example, used when we pull images
> from a Docker repository. The HTTP response of such a CONNECT request
> surfaced a problem with our HTTP parser, because it doesn't have neither
> any headers nor a message body. The parser expected a 'Content-Length'
> header to be set in every HTTP response. We fix this by assuming that
> a HTTP response doesn't have a message body when the 'Content-Length'
> header isn't set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> 87563f4fcdfaa2a33c4482533ddff24e062e603a 
> 
> Diff: https://reviews.apache.org/r/55496/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Benjamin Bannier]

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

(Updated Jan. 13, 2017, 4:50 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
---

Addressed review comment from guoger.


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


Repository: mesos


Description
---

This updates the resource reservation validation for frameworks which
can have multiple roles. During a deprecation period 'FrameworkInfo'
will have fields for both 'role' and 'roles', however the validation
function works with just an optional set of roles. Here an empty set
captures the previous semantics of either having an empty 'role' field
or 'role' set as '*'. This forces the callers to properly construct a
set of framework roles from the available information. An optional set
is used in order to accommodate callers which have no information
about the framework's roles, and ultimately disables validation taking
that information into account.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
  src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
  src/tests/master_validation_tests.cpp 
e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55461/diff/


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Benjamin Bannier]

> On Jan. 13, 2017, 9:17 a.m., Jay Guo wrote:
> > When a resource with `*` is offered to a multi-role framework, how does the 
> > framework decide which role to reserve the resource for?

Frameworks with default role cannot reserve resources; this is the first check 
in the validation function being updated in this patch.


- Benjamin


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


On Jan. 13, 2017, 4:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Jan. 13, 2017, 4:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Benjamin Bannier]

> On Jan. 13, 2017, 9:15 a.m., Jay Guo wrote:
> > src/master/master.cpp, lines 3944-3950
> > 
> >
> > I think we explicitly disallow empyt role field? 
> > https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
> > Also, should the case `framework->info.has_role() == false && 
> > framework->info.role() != "*"` be invalid and caught upon subscription?

Multi-role frameworks would use the `roles` field, in which case the `role` 
field would not be set. We need to distinguish here whether a framework uses 
`role` or `roles`. The check can be simplified to `framework->info.role() != 
"*"` though, independently of what we assume about this field upstream. I made 
that change.

Does that address your comment?


- Benjamin


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


On Jan. 13, 2017, 4:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Jan. 13, 2017, 4:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.

[Jan Schlicht]

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

(Updated Jan. 13, 2017, 4:51 p.m.)


Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

When behind a proxy, access to websites using SSL can be done by using
HTTP CONNECT tunneling. This is, for example, used when we pull images
from a Docker repository. The HTTP response of such a CONNECT request
surfaced a problem with our HTTP parser, because it doesn't have neither
any headers nor a message body. The parser expected a 'Content-Length'
header to be set in every HTTP response. We fix this by assuming that
a HTTP response doesn't have a message body when the 'Content-Length'
header isn't set.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
87563f4fcdfaa2a33c4482533ddff24e062e603a 

Diff: https://reviews.apache.org/r/55496/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 55480: Fix segfault when the executor sets a UUID that is not a valid v4 UUID.

[Aaron Wood]

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

(Updated Jan. 13, 2017, 3:52 p.m.)


Review request for mesos and Anand Mazumdar.


Summary (updated)
-

Fix segfault when the executor sets a UUID that is not a valid v4 UUID.


Repository: mesos


Description
---

This fixes the segfault that occurs when an executor sets a UUID that's not a 
valid v4 UUID and sends it off to the agent:

```
ABORT: (../../3rdparty/stout/include/stout/try.hpp:77): Try::get() but state == 
ERROR: Not a valid UUID
*** Aborted at 1484262968 (unix time) try "date -d @1484262968" if you are 
using GNU date ***
PC: @ 0x7efeb6101428 (unknown)
*** SIGABRT (@0x36b7) received by PID 14007 (TID 0x7efeabd29700) from PID 
14007; stack trace: ***
@ 0x7efeb64a6390 (unknown)
@ 0x7efeb6101428 (unknown)
@ 0x7efeb610302a (unknown)
@ 0x560df739fa6e _Abort()
@ 0x560df739fa9c _Abort()
@ 0x7efebb53a5ad Try<>::get()
@ 0x7efebb5363d6 Try<>::get()
@ 0x7efebbd84809 
mesos::internal::slave::validation::executor::call::validate()
@ 0x7efebbb59b36 mesos::internal::slave::Slave::Http::executor()
@ 0x7efebbc773b8 
_ZZN5mesos8internal5slave5Slave10initializeEvENKUlRKN7process4http7RequestEE1_clES7_
@ 0x7efebbcb5808 
_ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEERKNS2_7RequestEEZN5mesos8internal5slave5Slave10initializeEvEUlS7_E1_E9_M_invokeERKSt9_Any_dataS7_
@ 0x7efebbfb2aea std::function<>::operator()()
@ 0x7efebcb158b8 
_ZZZN7process11ProcessBase6_visitERKNS0_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSD_14authentication20AuthenticationResultEEE0_clESN_ENKUlbE1_clEb
@ 0x7efebcb1a10a 
_ZZZNK7process9_DeferredIZZNS_11ProcessBase6_visitERKNS1_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSE_14authentication20AuthenticationResultEEE0_clESO_EUlbE1_EcvSt8functionIFT_T0_EEINS_6FutureINSE_8ResponseEEERKbEEvENKUlS12_E_clES12_ENKUlvE_clEv
@ 0x7efebcb1c5f8 
_ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEEvEZZNKS0_9_DeferredIZZNS0_11ProcessBase6_visitERKNS7_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS0_5OwnedINS2_7RequestNKUlRK6OptionINS2_14authentication20AuthenticationResultEEE0_clEST_EUlbE1_EcvSt8functionIFT_T0_EEIS4_RKbEEvENKUlS14_E_clES14_EUlvE_E9_M_invokeERKSt9_Any_data
@ 0x7efebb5ce8ca std::function<>::operator()()
@ 0x7efebb5c4b27 
_ZZN7process8internal8DispatchINS_6FutureINS_4http8ResponseclIRSt8functionIFS5_vS5_RKNS_4UPIDEOT_ENKUlPNS_11ProcessBaseEE_clESI_
@ 0x7efebb5d4e1e 
_ZNSt17_Function_handlerIFvPN7process11ProcessBaseEEZNS0_8internal8DispatchINS0_6FutureINS0_4http8ResponseclIRSt8functionIFS9_vS9_RKNS0_4UPIDEOT_EUlS2_E_E9_M_invokeERKSt9_Any_dataOS2_
@ 0x7efebcb30baf std::function<>::operator()()
@ 0x7efebcb13fd6 process::ProcessBase::visit()
@ 0x7efebcb1f3c8 process::DispatchEvent::visit()
@ 0x7efebb3ab2ea process::ProcessBase::serve()
@ 0x7efebcb0fe8a process::ProcessManager::resume()
@ 0x7efebcb0c5a3 _ZZN7process14ProcessManager12init_threadsEvENKUt_clEv
@ 0x7efebcb1ea34 
_ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEE9_M_invokeIJEEEvSt12_Index_tupleIJXspT_EEE
@ 0x7efebcb1e98a 
_ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEclEv
@ 0x7efebcb1e91a 
_ZNSt6thread5_ImplISt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEE6_M_runEv
@ 0x7efeb6980c80 (unknown)
@ 0x7efeb649c6ba start_thread
@ 0x7efeb61d282d (unknown)
Aborted (core dumped)
```


Diffs
-

  src/slave/validation.cpp abd9b1248 

Diff: https://reviews.apache.org/r/55480/diff/


Testing
---

Verified by making sure no segfault occurs when rebuilding Mesos with this fix 
and pointing our framework at it. Our framework is currently not generating v4 
UUIDs which was exposing the issue.


Thanks,

Aaron Wood

Re: Review Request 55480: Fix segfault when the executor sets a UUID that is not a valid v4 UUID.

[Aaron Wood]

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

(Updated Jan. 13, 2017, 3:56 p.m.)


Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

This fixes the segfault that occurs when an executor sets a UUID that's not a 
valid v4 UUID and sends it off to the agent:

```
ABORT: (../../3rdparty/stout/include/stout/try.hpp:77): Try::get() but state == 
ERROR: Not a valid UUID
*** Aborted at 1484262968 (unix time) try "date -d @1484262968" if you are 
using GNU date ***
PC: @ 0x7efeb6101428 (unknown)
*** SIGABRT (@0x36b7) received by PID 14007 (TID 0x7efeabd29700) from PID 
14007; stack trace: ***
@ 0x7efeb64a6390 (unknown)
@ 0x7efeb6101428 (unknown)
@ 0x7efeb610302a (unknown)
@ 0x560df739fa6e _Abort()
@ 0x560df739fa9c _Abort()
@ 0x7efebb53a5ad Try<>::get()
@ 0x7efebb5363d6 Try<>::get()
@ 0x7efebbd84809 
mesos::internal::slave::validation::executor::call::validate()
@ 0x7efebbb59b36 mesos::internal::slave::Slave::Http::executor()
@ 0x7efebbc773b8 
_ZZN5mesos8internal5slave5Slave10initializeEvENKUlRKN7process4http7RequestEE1_clES7_
@ 0x7efebbcb5808 
_ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEERKNS2_7RequestEEZN5mesos8internal5slave5Slave10initializeEvEUlS7_E1_E9_M_invokeERKSt9_Any_dataS7_
@ 0x7efebbfb2aea std::function<>::operator()()
@ 0x7efebcb158b8 
_ZZZN7process11ProcessBase6_visitERKNS0_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSD_14authentication20AuthenticationResultEEE0_clESN_ENKUlbE1_clEb
@ 0x7efebcb1a10a 
_ZZZNK7process9_DeferredIZZNS_11ProcessBase6_visitERKNS1_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSE_14authentication20AuthenticationResultEEE0_clESO_EUlbE1_EcvSt8functionIFT_T0_EEINS_6FutureINSE_8ResponseEEERKbEEvENKUlS12_E_clES12_ENKUlvE_clEv
@ 0x7efebcb1c5f8 
_ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEEvEZZNKS0_9_DeferredIZZNS0_11ProcessBase6_visitERKNS7_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS0_5OwnedINS2_7RequestNKUlRK6OptionINS2_14authentication20AuthenticationResultEEE0_clEST_EUlbE1_EcvSt8functionIFT_T0_EEIS4_RKbEEvENKUlS14_E_clES14_EUlvE_E9_M_invokeERKSt9_Any_data
@ 0x7efebb5ce8ca std::function<>::operator()()
@ 0x7efebb5c4b27 
_ZZN7process8internal8DispatchINS_6FutureINS_4http8ResponseclIRSt8functionIFS5_vS5_RKNS_4UPIDEOT_ENKUlPNS_11ProcessBaseEE_clESI_
@ 0x7efebb5d4e1e 
_ZNSt17_Function_handlerIFvPN7process11ProcessBaseEEZNS0_8internal8DispatchINS0_6FutureINS0_4http8ResponseclIRSt8functionIFS9_vS9_RKNS0_4UPIDEOT_EUlS2_E_E9_M_invokeERKSt9_Any_dataOS2_
@ 0x7efebcb30baf std::function<>::operator()()
@ 0x7efebcb13fd6 process::ProcessBase::visit()
@ 0x7efebcb1f3c8 process::DispatchEvent::visit()
@ 0x7efebb3ab2ea process::ProcessBase::serve()
@ 0x7efebcb0fe8a process::ProcessManager::resume()
@ 0x7efebcb0c5a3 _ZZN7process14ProcessManager12init_threadsEvENKUt_clEv
@ 0x7efebcb1ea34 
_ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEE9_M_invokeIJEEEvSt12_Index_tupleIJXspT_EEE
@ 0x7efebcb1e98a 
_ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEclEv
@ 0x7efebcb1e91a 
_ZNSt6thread5_ImplISt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEE6_M_runEv
@ 0x7efeb6980c80 (unknown)
@ 0x7efeb649c6ba start_thread
@ 0x7efeb61d282d (unknown)
Aborted (core dumped)
```


Diffs
-

  src/slave/validation.cpp abd9b1248 

Diff: https://reviews.apache.org/r/55480/diff/


Testing
---

Verified by making sure no segfault occurs when rebuilding Mesos with this fix 
and pointing our framework at it. Our framework is currently not generating v4 
UUIDs which was exposing the issue.


Thanks,

Aaron Wood

Re: Review Request 55480: Fix segfault when the executor sets a UUID that is not a valid v4 UUID.

[Aaron Wood]

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

(Updated Jan. 13, 2017, 4:46 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Adjusted one of the tests to set a bad UUID value.


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


Repository: mesos


Description
---

This fixes the segfault that occurs when an executor sets a UUID that's not a 
valid v4 UUID and sends it off to the agent:

```
ABORT: (../../3rdparty/stout/include/stout/try.hpp:77): Try::get() but state == 
ERROR: Not a valid UUID
*** Aborted at 1484262968 (unix time) try "date -d @1484262968" if you are 
using GNU date ***
PC: @ 0x7efeb6101428 (unknown)
*** SIGABRT (@0x36b7) received by PID 14007 (TID 0x7efeabd29700) from PID 
14007; stack trace: ***
@ 0x7efeb64a6390 (unknown)
@ 0x7efeb6101428 (unknown)
@ 0x7efeb610302a (unknown)
@ 0x560df739fa6e _Abort()
@ 0x560df739fa9c _Abort()
@ 0x7efebb53a5ad Try<>::get()
@ 0x7efebb5363d6 Try<>::get()
@ 0x7efebbd84809 
mesos::internal::slave::validation::executor::call::validate()
@ 0x7efebbb59b36 mesos::internal::slave::Slave::Http::executor()
@ 0x7efebbc773b8 
_ZZN5mesos8internal5slave5Slave10initializeEvENKUlRKN7process4http7RequestEE1_clES7_
@ 0x7efebbcb5808 
_ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEERKNS2_7RequestEEZN5mesos8internal5slave5Slave10initializeEvEUlS7_E1_E9_M_invokeERKSt9_Any_dataS7_
@ 0x7efebbfb2aea std::function<>::operator()()
@ 0x7efebcb158b8 
_ZZZN7process11ProcessBase6_visitERKNS0_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSD_14authentication20AuthenticationResultEEE0_clESN_ENKUlbE1_clEb
@ 0x7efebcb1a10a 
_ZZZNK7process9_DeferredIZZNS_11ProcessBase6_visitERKNS1_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSE_14authentication20AuthenticationResultEEE0_clESO_EUlbE1_EcvSt8functionIFT_T0_EEINS_6FutureINSE_8ResponseEEERKbEEvENKUlS12_E_clES12_ENKUlvE_clEv
@ 0x7efebcb1c5f8 
_ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEEvEZZNKS0_9_DeferredIZZNS0_11ProcessBase6_visitERKNS7_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS0_5OwnedINS2_7RequestNKUlRK6OptionINS2_14authentication20AuthenticationResultEEE0_clEST_EUlbE1_EcvSt8functionIFT_T0_EEIS4_RKbEEvENKUlS14_E_clES14_EUlvE_E9_M_invokeERKSt9_Any_data
@ 0x7efebb5ce8ca std::function<>::operator()()
@ 0x7efebb5c4b27 
_ZZN7process8internal8DispatchINS_6FutureINS_4http8ResponseclIRSt8functionIFS5_vS5_RKNS_4UPIDEOT_ENKUlPNS_11ProcessBaseEE_clESI_
@ 0x7efebb5d4e1e 
_ZNSt17_Function_handlerIFvPN7process11ProcessBaseEEZNS0_8internal8DispatchINS0_6FutureINS0_4http8ResponseclIRSt8functionIFS9_vS9_RKNS0_4UPIDEOT_EUlS2_E_E9_M_invokeERKSt9_Any_dataOS2_
@ 0x7efebcb30baf std::function<>::operator()()
@ 0x7efebcb13fd6 process::ProcessBase::visit()
@ 0x7efebcb1f3c8 process::DispatchEvent::visit()
@ 0x7efebb3ab2ea process::ProcessBase::serve()
@ 0x7efebcb0fe8a process::ProcessManager::resume()
@ 0x7efebcb0c5a3 _ZZN7process14ProcessManager12init_threadsEvENKUt_clEv
@ 0x7efebcb1ea34 
_ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEE9_M_invokeIJEEEvSt12_Index_tupleIJXspT_EEE
@ 0x7efebcb1e98a 
_ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEclEv
@ 0x7efebcb1e91a 
_ZNSt6thread5_ImplISt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEE6_M_runEv
@ 0x7efeb6980c80 (unknown)
@ 0x7efeb649c6ba start_thread
@ 0x7efeb61d282d (unknown)
Aborted (core dumped)
```


Diffs (updated)
-

  src/slave/validation.cpp abd9b12 
  src/tests/executor_http_api_tests.cpp 872caf6 

Diff: https://reviews.apache.org/r/55480/diff/


Testing
---

Verified by making sure no segfault occurs when rebuilding Mesos with this fix 
and pointing our framework at it. Our framework is currently not generating v4 
UUIDs which was exposing the issue.


Thanks,

Aaron Wood

Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[James Peach]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

To be consistent with Master::statusUpdateAcknowledgement(), validate
that the UUID in the StatusUpdate message is acceptable to the
UUID::fromBytes() parser.


Diffs
-

  3rdparty/stout/include/stout/os/linux.hpp 
68b4090aa5e6f23609c487128d91301755ae0e46 
  docs/contributors.yaml d2b686c9c15782eefeed3376dc84edba22968745 
  src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691 
  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/slave/containerizer/mesos/containerizer.cpp 
4d2b4344e9698aed6ef0d49bb56d9292f2c3703b 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
  support/apply-reviews.py c77b4c2b2d7a3d5e74e225403e71a84e23a9a1e7 

Diff: https://reviews.apache.org/r/55509/diff/


Testing
---

Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.


Thanks,

James Peach

Re: Review Request 55487: Renamed executable files from '_' to '-' in the `support` directory.

[Vinod Kone]

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


Ship it!




Make sure you disable the ASF CI jobs before you land this. Re-enable them 
after updating the Jenkins job configurations.

- Vinod Kone


On Jan. 13, 2017, 9:54 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55487/
> ---
> 
> (Updated Jan. 13, 2017, 9:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh  
>   support/hooks/post-rewrite e3747a320fef0b71c06bcf0f2c5532958c416646 
>   support/hooks/pre-commit 10838a4c99db2a8318d64f95d90d2c2c90150453 
>   support/mesos_split.py  
>   support/timed_tests.sh  
>   support/verify_reviews.py  
> 
> Diff: https://reviews.apache.org/r/55487/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[Neil Conway]

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



Seems like you need to rebase?

- Neil Conway


On Jan. 13, 2017, 5:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55509/
> ---
> 
> (Updated Jan. 13, 2017, 5:22 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6920
> https://issues.apache.org/jira/browse/MESOS-6920
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be consistent with Master::statusUpdateAcknowledgement(), validate
> that the UUID in the StatusUpdate message is acceptable to the
> UUID::fromBytes() parser.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/linux.hpp 
> 68b4090aa5e6f23609c487128d91301755ae0e46 
>   docs/contributors.yaml d2b686c9c15782eefeed3376dc84edba22968745 
>   src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4d2b4344e9698aed6ef0d49bb56d9292f2c3703b 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
>   support/apply-reviews.py c77b4c2b2d7a3d5e74e225403e71a84e23a9a1e7 
> 
> Diff: https://reviews.apache.org/r/55509/diff/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[James Peach]

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

(Updated Jan. 13, 2017, 6:41 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased onto master.


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


Repository: mesos


Description
---

To be consistent with Master::statusUpdateAcknowledgement(), validate
that the UUID in the StatusUpdate message is acceptable to the
UUID::fromBytes() parser.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 

Diff: https://reviews.apache.org/r/55509/diff/


Testing
---

Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.


Thanks,

James Peach

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[James Peach]

> On Jan. 13, 2017, 6:39 p.m., Neil Conway wrote:
> > Seems like you need to rebase?

Oops, thanks :)


- James


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


On Jan. 13, 2017, 6:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55509/
> ---
> 
> (Updated Jan. 13, 2017, 6:41 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6920
> https://issues.apache.org/jira/browse/MESOS-6920
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be consistent with Master::statusUpdateAcknowledgement(), validate
> that the UUID in the StatusUpdate message is acceptable to the
> UUID::fromBytes() parser.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55509/diff/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55446: Added common validation for IDs.

[Anindya Sinha]

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




src/common/validation.cpp (line 36)


Should we consider disallowing spaces as well? `iscntrl` includes tabs but 
do we want spaces to be included in the path?


- Anindya Sinha


On Jan. 12, 2017, 9:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55446/
> ---
> 
> (Updated Jan. 12, 2017, 9:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-6866
> https://issues.apache.org/jira/browse/MESOS-6866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - The common validation logic is shared by the master and the agent.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c 
>   src/Makefile.am 333b45683a10eaac3b653e006511306d8054922c 
>   src/common/validation.hpp PRE-CREATION 
>   src/common/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55446/diff/
> 
> 
> Testing
> ---
> 
> make check
> Tested with /r/55449/
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55487: Renamed executable files from '_' to '-' in the `support` directory.

[Michael Park]

> On Jan. 13, 2017, 10:13 a.m., Vinod Kone wrote:
> > Make sure you disable the ASF CI jobs before you land this. Re-enable them 
> > after updating the Jenkins job configurations.

Yep!


- Michael


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


On Jan. 13, 2017, 1:54 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55487/
> ---
> 
> (Updated Jan. 13, 2017, 1:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh  
>   support/hooks/post-rewrite e3747a320fef0b71c06bcf0f2c5532958c416646 
>   support/hooks/pre-commit 10838a4c99db2a8318d64f95d90d2c2c90150453 
>   support/mesos_split.py  
>   support/timed_tests.sh  
>   support/verify_reviews.py  
> 
> Diff: https://reviews.apache.org/r/55487/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55238: Use os::spawn in the CNI isolator.

[Jiang Yan Xu]

> On Jan. 11, 2017, 2:11 a.m., Jiang Yan Xu wrote:
> > I feel if we keep `os::system()` in the codebase at all, this is one of the 
> > few places it could actually be used... we could eliminate it so we can say 
> > there's no references to `os::systems()` left today but it's a bit harsh to 
> > nit-picking on future use like this in order to keep a "clean state"?
> 
> James Peach wrote:
> I don't consider this harsh, just a minor, obvious improvement. While 
> `system()` is safe, `spawn()` is slightly better because it doesn't use the 
> shell. We can't completely eliminate `system()` because there are places that 
> actually require the shell (eg. the port mapping CNI plugin).
> 
> Jiang Yan Xu wrote:
> OK I guess if the command is a static string and trivial enough *and some 
> shell features make it easier to write* then os::system still fine. For this 
> case yes we can replace it.
> 
> Avinash sridharan wrote:
> As long as `ifconfig` is guaranteed to be an elf binary and not a script 
> I think this should be ok. Looked at a few distributions and seems like that 
> is the case.

Why not a script? I tried to use os::spawn() with a script and it worked? Do 
you mean shell builtin commands?


- Jiang Yan


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


On Jan. 11, 2017, 4:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55238/
> ---
> 
> (Updated Jan. 11, 2017, 4:26 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use os::spawn in the CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
> 
> Diff: https://reviews.apache.org/r/55238/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55446: Added common validation for IDs.

[Jiang Yan Xu]

> On Jan. 13, 2017, 10:50 a.m., Anindya Sinha wrote:
> > src/common/validation.cpp, line 36
> > 
> >
> > Should we consider disallowing spaces as well? `iscntrl` includes tabs 
> > but do we want spaces to be included in the path?

IMO we should but we currently don't. This patch doesn't propose new rules. I 
was going to create a JIRA for some new rules but JIRA is down. :(


- Jiang Yan


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


On Jan. 12, 2017, 1:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55446/
> ---
> 
> (Updated Jan. 12, 2017, 1:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-6866
> https://issues.apache.org/jira/browse/MESOS-6866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - The common validation logic is shared by the master and the agent.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c 
>   src/Makefile.am 333b45683a10eaac3b653e006511306d8054922c 
>   src/common/validation.hpp PRE-CREATION 
>   src/common/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55446/diff/
> 
> 
> Testing
> ---
> 
> make check
> Tested with /r/55449/
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55238: Use os::spawn in the CNI isolator.

[Avinash sridharan]

> On Jan. 11, 2017, 10:11 a.m., Jiang Yan Xu wrote:
> > I feel if we keep `os::system()` in the codebase at all, this is one of the 
> > few places it could actually be used... we could eliminate it so we can say 
> > there's no references to `os::systems()` left today but it's a bit harsh to 
> > nit-picking on future use like this in order to keep a "clean state"?
> 
> James Peach wrote:
> I don't consider this harsh, just a minor, obvious improvement. While 
> `system()` is safe, `spawn()` is slightly better because it doesn't use the 
> shell. We can't completely eliminate `system()` because there are places that 
> actually require the shell (eg. the port mapping CNI plugin).
> 
> Jiang Yan Xu wrote:
> OK I guess if the command is a static string and trivial enough *and some 
> shell features make it easier to write* then os::system still fine. For this 
> case yes we can replace it.
> 
> Avinash sridharan wrote:
> As long as `ifconfig` is guaranteed to be an elf binary and not a script 
> I think this should be ok. Looked at a few distributions and seems like that 
> is the case.
> 
> Jiang Yan Xu wrote:
> Why not a script? I tried to use os::spawn() with a script and it worked? 
> Do you mean shell builtin commands?

My bad, please ignore my comment. I didn't realize we are not using 
`posix_spawn` internally. Should have verified the implementation.


- Avinash


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


On Jan. 12, 2017, 12:26 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55238/
> ---
> 
> (Updated Jan. 12, 2017, 12:26 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use os::spawn in the CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
> 
> Diff: https://reviews.apache.org/r/55238/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55343: Fixed SSL socket 'shutdown()'.

[Greg Mann]

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

(Updated Jan. 13, 2017, 7:19 p.m.)


Review request for mesos, Benjamin Hindman and Joseph Wu.


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


Repository: mesos


Description
---

Recently, a change was made to the signature of
`Socket::shutdown`, but the corresponding override in
`LibeventSSLSocketImpl` was not updated, so that the
implementation-specific method is no longer being
executed. Further, the SSL socket's `shutdown` code
did not actually shutdown the socket; rather, the
shutdown was performed in the destructor.

This patch updates the function's signature to match
that of the base class's method, adds the `override`
specifier to the implemention's method declaration,
and updates the function to properly shutdown the
SSL socket.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 
87966155aa21328db51796b2ae0a883054c00457 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
65da091155107d77bdf7b003609ab3770f80083a 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
b0319b2d3694f600190615ba6d29b95b1d8f2405 

Diff: https://reviews.apache.org/r/55343/diff/


Testing
---

`3rdparty/libprocess/libprocess-tests 
--gtest_filter="Encryption/NetSocketTest.EOFBeforeRecv/0:Encryption/NetSocketTest.EOFAfterRecv/0"
 --gtest_repeat=-1 --gtest_break_on_failure`

NOTE: currently the above command exposes a file descriptor leak related to the 
SSL socket.


Thanks,

Greg Mann

Re: Review Request 55448: Implemented TODOs to use common ID validation.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On Jan. 12, 2017, 9:48 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55448/
> ---
> 
> (Updated Jan. 12, 2017, 9:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented TODOs to use common ID validation.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/slave/validation.cpp abd9b12483a9a62a40f6b6e89c5d866c9f8881af 
> 
> Diff: https://reviews.apache.org/r/55448/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55480: Fix segfault when the executor sets a UUID that is not a valid v4 UUID.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55480]

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 Jan. 13, 2017, 4:46 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55480/
> ---
> 
> (Updated Jan. 13, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6917
> https://issues.apache.org/jira/browse/MESOS-6917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the segfault that occurs when an executor sets a UUID that's not a 
> valid v4 UUID and sends it off to the agent:
> 
> ```
> ABORT: (../../3rdparty/stout/include/stout/try.hpp:77): Try::get() but state 
> == ERROR: Not a valid UUID
> *** Aborted at 1484262968 (unix time) try "date -d @1484262968" if you are 
> using GNU date ***
> PC: @ 0x7efeb6101428 (unknown)
> *** SIGABRT (@0x36b7) received by PID 14007 (TID 0x7efeabd29700) from PID 
> 14007; stack trace: ***
> @ 0x7efeb64a6390 (unknown)
> @ 0x7efeb6101428 (unknown)
> @ 0x7efeb610302a (unknown)
> @ 0x560df739fa6e _Abort()
> @ 0x560df739fa9c _Abort()
> @ 0x7efebb53a5ad Try<>::get()
> @ 0x7efebb5363d6 Try<>::get()
> @ 0x7efebbd84809 
> mesos::internal::slave::validation::executor::call::validate()
> @ 0x7efebbb59b36 mesos::internal::slave::Slave::Http::executor()
> @ 0x7efebbc773b8 
> _ZZN5mesos8internal5slave5Slave10initializeEvENKUlRKN7process4http7RequestEE1_clES7_
> @ 0x7efebbcb5808 
> _ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEERKNS2_7RequestEEZN5mesos8internal5slave5Slave10initializeEvEUlS7_E1_E9_M_invokeERKSt9_Any_dataS7_
> @ 0x7efebbfb2aea std::function<>::operator()()
> @ 0x7efebcb158b8 
> _ZZZN7process11ProcessBase6_visitERKNS0_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSD_14authentication20AuthenticationResultEEE0_clESN_ENKUlbE1_clEb
> @ 0x7efebcb1a10a 
> _ZZZNK7process9_DeferredIZZNS_11ProcessBase6_visitERKNS1_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS_5OwnedINS_4http7RequestNKUlRK6OptionINSE_14authentication20AuthenticationResultEEE0_clESO_EUlbE1_EcvSt8functionIFT_T0_EEINS_6FutureINSE_8ResponseEEERKbEEvENKUlS12_E_clES12_ENKUlvE_clEv
> @ 0x7efebcb1c5f8 
> _ZNSt17_Function_handlerIFN7process6FutureINS0_4http8ResponseEEEvEZZNKS0_9_DeferredIZZNS0_11ProcessBase6_visitERKNS7_12HttpEndpointERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS0_5OwnedINS2_7RequestNKUlRK6OptionINS2_14authentication20AuthenticationResultEEE0_clEST_EUlbE1_EcvSt8functionIFT_T0_EEIS4_RKbEEvENKUlS14_E_clES14_EUlvE_E9_M_invokeERKSt9_Any_data
> @ 0x7efebb5ce8ca std::function<>::operator()()
> @ 0x7efebb5c4b27 
> _ZZN7process8internal8DispatchINS_6FutureINS_4http8ResponseclIRSt8functionIFS5_vS5_RKNS_4UPIDEOT_ENKUlPNS_11ProcessBaseEE_clESI_
> @ 0x7efebb5d4e1e 
> _ZNSt17_Function_handlerIFvPN7process11ProcessBaseEEZNS0_8internal8DispatchINS0_6FutureINS0_4http8ResponseclIRSt8functionIFS9_vS9_RKNS0_4UPIDEOT_EUlS2_E_E9_M_invokeERKSt9_Any_dataOS2_
> @ 0x7efebcb30baf std::function<>::operator()()
> @ 0x7efebcb13fd6 process::ProcessBase::visit()
> @ 0x7efebcb1f3c8 process::DispatchEvent::visit()
> @ 0x7efebb3ab2ea process::ProcessBase::serve()
> @ 0x7efebcb0fe8a process::ProcessManager::resume()
> @ 0x7efebcb0c5a3 
> _ZZN7process14ProcessManager12init_threadsEvENKUt_clEv
> @ 0x7efebcb1ea34 
> _ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEE9_M_invokeIJEEEvSt12_Index_tupleIJXspT_EEE
> @ 0x7efebcb1e98a 
> _ZNSt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEclEv
> @ 0x7efebcb1e91a 
> _ZNSt6thread5_ImplISt12_Bind_simpleIFZN7process14ProcessManager12init_threadsEvEUt_vEEE6_M_runEv
> @ 0x7efeb6980c80 (unknown)
> @ 0x7efeb649c6ba start_thread
> @ 0x7efeb61d282d (unknown)
> Aborted (core dumped)
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/validation.cpp abd9b12 
>   src/tests/executor_http_api_tests.cpp 872caf6 
> 
> Diff: https://reviews.apache.org/r/55480/diff/
> 
> 
> Testing
> ---
> 
> Verified by making sure no segfault occurs when rebuilding Mesos with this 
> fix and pointing our framework at it. Our framework is currently not 
> generating v4 UUIDs which was exposing the issue

Re: Review Request 55446: Added common validation for IDs.

[Jiang Yan Xu]

> On Jan. 12, 2017, 1:43 p.m., James Peach wrote:
> > src/common/validation.hpp, line 34
> > 
> >
> > Since this is never used externally, consider making it a local static 
> > helper/

This is used externally.


> On Jan. 12, 2017, 1:43 p.m., James Peach wrote:
> > src/common/validation.hpp, line 18
> > 
> >
> > Should this be `__COMMON_VALIDATION_HPP__`?

It's not a requirement to match the file path but since I am putting the 
methods in the `common` namespace I guess I could use 
`__COMMON_VALIDATION_HPP__` here.

Another point is that perhaps we could move *some* of these (as a TODO) to 
`/include/mesos/validation.hpp` for which we could use 
`__VALIDATION_HPP__`.


- Jiang Yan


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


On Jan. 12, 2017, 1:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55446/
> ---
> 
> (Updated Jan. 12, 2017, 1:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-6866
> https://issues.apache.org/jira/browse/MESOS-6866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - The common validation logic is shared by the master and the agent.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c 
>   src/Makefile.am 333b45683a10eaac3b653e006511306d8054922c 
>   src/common/validation.hpp PRE-CREATION 
>   src/common/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55446/diff/
> 
> 
> Testing
> ---
> 
> make check
> Tested with /r/55449/
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55447: Added sanity checks on IDs and roles before creating directories.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On Jan. 12, 2017, 9:48 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55447/
> ---
> 
> (Updated Jan. 12, 2017, 9:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-6866
> https://issues.apache.org/jira/browse/MESOS-6866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The context for adding these is MESOS-6866. Addmittedly these checks only 
> protect against a (small) subset of potential attacks by clients spoofing as 
> the master and we need MESOS-6903 to truely fix the issue. However instead of 
> ad-hoc bandaids, these could be considered good programming practices: CHECK 
> preconditions before performing (potentially dangerous) actions so I think 
> this is still reasonable.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.cpp 8792cee43d94e7b0bbd7b80aebbe501236244621 
> 
> Diff: https://reviews.apache.org/r/55447/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 55521: Handle perf versions with more than 3 components.

[James Peach]

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

Review request for mesos, Ian Downes, Kapil Arya, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Handle perf versions with more than 3 components.


Diffs
-

  src/linux/perf.hpp c7dfe0ac4965a450b25085715502f49bee972b5c 
  src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 
  src/tests/containerizer/perf_tests.cpp 
e5ee3e71478e167c345e4a9a7c2c306d302b028b 

Diff: https://reviews.apache.org/r/55521/diff/


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 55521: Handle perf versions with more than 3 components.

[James Peach]

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

(Updated Jan. 13, 2017, 8:24 p.m.)


Review request for mesos, Ian Downes, Kapil Arya, and Jiang Yan Xu.


Bugs: MESOS-6552 and MESOS-6867
https://issues.apache.org/jira/browse/MESOS-6552
https://issues.apache.org/jira/browse/MESOS-6867


Repository: mesos


Description
---

Handle perf versions with more than 3 components.


Diffs
-

  src/linux/perf.hpp c7dfe0ac4965a450b25085715502f49bee972b5c 
  src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 
  src/tests/containerizer/perf_tests.cpp 
e5ee3e71478e167c345e4a9a7c2c306d302b028b 

Diff: https://reviews.apache.org/r/55521/diff/


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55509]

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 Jan. 13, 2017, 6:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55509/
> ---
> 
> (Updated Jan. 13, 2017, 6:41 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6920
> https://issues.apache.org/jira/browse/MESOS-6920
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be consistent with Master::statusUpdateAcknowledgement(), validate
> that the UUID in the StatusUpdate message is acceptable to the
> UUID::fromBytes() parser.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55509/diff/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55021: Added a framework capabilities struct.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good, just a few minor suggestions and we're good to go. I don't think 
the test here was critical given that any code relying on the capability will 
fail, but I left a comment about how we could test this more comprehensively 
without making the test much longer.


src/common/protobuf_utils.hpp (line 207)


Brace on a newline.



src/common/protobuf_utils.hpp (lines 210 - 212)


Is it possible for this to be templated to allow any iterable? As we did 
here:

https://github.com/apache/mesos/blob/104532d867214b6d3c6fe96d6ebb144aa8803f7c/3rdparty/stout/include/stout/strings.hpp#L332-L334

```
  template 
  Capabilities(const Capabilities& capabilities)
  {
foreach (const FrameworkInfo::Capability& capability, capabilities) {
  ...
}
  }
```

This lets us test this a bit more easily (see below).



src/common/protobuf_utils.hpp (line 214)


Since FramworkInfo::Capability is not a POD type, let's leave the reference 
here, so:

```
foreach (const FrameworkInfo::Capability& capability, capabilities) {
```

Note that while FrameworkInfo::Capability currently only has an enum 
currently, it can grow to be more expensive to copy in the future.



src/tests/protobuf_utils_tests.cpp (lines 63 - 85)


The current test seems a bit arbitrary, perhaps we could test this a bit 
more comprehensively using a reverse operation and equality? Something like:

```
TEST(ProtobufUtilTest, Capabilities)
{
  auto toSet = [](const protobuf::framework::Capabilities& capabilities) {
set result;

if (capabilities.revocableResources) {
  result.insert(FrameworkInfo::Capability::REVOCABLE_RESOURCES);
}
if (capabilities.taskKillingState) {
  result.insert(FrameworkInfo::Capability::TASK_KILLING_STATE);
}
if (capabilities.gpuResources) {
  result.insert(FrameworkInfo::Capability::GPU_RESOURCES);  
}
if (capabilities.sharedResources) {
  result.insert(FrameworkInfo::Capability::SHARED_RESOURCES);
}
if (capabilities.partitionAware) {
  result.insert(FrameworkInfo::Capability::PARTITION_AWARE);
}
if (capabilities.multiRole) {
  result.insert(FrameworkInfo::Capability::MULTI_ROLE);
}

return result;
  };

  set expected = { REVOCABLE_RESOURCES };
  EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
  
  expected = { TASK_KILLING_STATE };
  EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
  
  expected = { GPU_RESOURCES };
  EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
  
  expected = { SHARED_RESOURCES };
  EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
  
  expected = { PARTITION_AWARE };
  EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
  
  expected = { MULTI_ROLE };
  EXPECT_EQ(expected, toSet(protobuf::framework::Capabilities(expected));
}
```

This isn't a lot more code but it tests it more comprehensively.


- Benjamin Mahler


On Jan. 6, 2017, 7:21 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> ---
> 
> (Updated Jan. 6, 2017, 7:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60 
>   src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723 
> 
> Diff: https://reviews.apache.org/r/55021/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55066: Added Capabilities to Framework struct of Master and Agent.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/http.cpp (line 236)


Can you fix this separately?


- Benjamin Mahler


On Jan. 5, 2017, 12:19 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55066/
> ---
> 
> (Updated Jan. 5, 2017, 12:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When Framework struct is constructed, Capabilities is initialized
> with capabilities in FrameworkInfo. Some of `frameworkHasCapability`
> are then retired in favor of checking boolean value in Capabilities.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 75dcd6199dbfcca6260baed49b838a45312bb667 
>   src/master/master.hpp 57fc6e6f2995078df80f0aa52707727db802ede0 
>   src/master/master.cpp 11c34a048586d30c6ac67be8638ed8fa81cc3f1f 
>   src/slave/http.cpp b6e2d4f9190358d113b39140d116b8659ddbb49b 
>   src/slave/slave.hpp acb4f6581fcf22f9a748f6ccea48665542e76bc3 
>   src/slave/slave.cpp f8f2ccfadb9a00be17c0b552586aa5875b7cbb19 
> 
> Diff: https://reviews.apache.org/r/55066/diff/
> 
> 
> Testing
> ---
> 
> make check on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55068: Added Capabilities to Framework struct of Hierarchical allocator.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 5, 2017, 12:19 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55068/
> ---
> 
> (Updated Jan. 5, 2017, 12:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Individual capabilities are collected into this struct and used in
> allocator. This is step toward multi-role support of allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> a6424d624864155e1c87a28a63b784512c5c8722 
>   src/master/allocator/mesos/hierarchical.cpp 
> 91b1ec43940a788459f045ca4a4b82d4e8373bca 
> 
> Diff: https://reviews.apache.org/r/55068/diff/
> 
> 
> Testing
> ---
> 
> make check on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55021: Added a framework capabilities struct.

[Benjamin Mahler]

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




src/tests/protobuf_utils_tests.cpp (line 65)


Also, let's call this 'FrameworkCapabilties' since we're planning to 
introduce capabilities for different components.


- Benjamin Mahler


On Jan. 6, 2017, 7:21 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> ---
> 
> (Updated Jan. 6, 2017, 7:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60 
>   src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723 
> 
> Diff: https://reviews.apache.org/r/55021/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Looks like there are a few more cases of status update acknowledgements that 
have this UUID issue.


src/master/master.cpp (line 5824)


Actually preincrement is the more common/style-guide-conforming style in 
Mesos and in the rest of the file, it's the other instances in this method that 
are inconsistent.


https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement



src/master/master.cpp (lines 5829 - 5830)


About

> ... from agent pid  with id ...

If we move this check down to be after 

```
Slave* slave = slaves.registered.get(update.slave_id());

if (slave == nullptr) {
...
}
```

Then you can just use the output overload to do

```
"agent " << *slave << ...;
```

which prints the agent info more uniformly.


- Jiang Yan Xu


On Jan. 13, 2017, 10:41 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55509/
> ---
> 
> (Updated Jan. 13, 2017, 10:41 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6920
> https://issues.apache.org/jira/browse/MESOS-6920
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be consistent with Master::statusUpdateAcknowledgement(), validate
> that the UUID in the StatusUpdate message is acceptable to the
> UUID::fromBytes() parser.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55509/diff/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55238: Use os::spawn in the CNI isolator.

[James Peach]

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

(Updated Jan. 13, 2017, 11:11 p.m.)


Review request for mesos, Avinash sridharan and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Use os::spawn in the CNI isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 

Diff: https://reviews.apache.org/r/55238/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55241: Stop using os::system to validate perf event names.

[James Peach]

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

(Updated Jan. 13, 2017, 11:11 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Stop using os::system to validate perf event names.


Diffs (updated)
-

  src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 

Diff: https://reviews.apache.org/r/55241/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55240: Stop using os::system to copy local files.

[James Peach]

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

(Updated Jan. 13, 2017, 11:11 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Stop using os::system to copy local files.


Diffs (updated)
-

  src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 

Diff: https://reviews.apache.org/r/55240/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55239: Stop using os::system to extract fetcher archives.

[James Peach]

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

(Updated Jan. 13, 2017, 11:11 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Stop using os::system to extract fetcher archives.


Diffs (updated)
-

  src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 

Diff: https://reviews.apache.org/r/55239/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[James Peach]

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

(Updated Jan. 13, 2017, 11:12 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Reimplement os::chown() to use fts(3) rather than sometimes spawning
chown(1). This removes the use of the shell and the corresponding
need to sanitize path arguments.  It also enables us to provide
consistent handling of symbolic links for the recursive and
non-recursive cases. We ensure that symlinks are never followed
and that we always change the ownership of the link itself, not
its referent.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/chown.hpp 
c82e2e574019c5ee5f17ea105a6d225006388a45 
  3rdparty/stout/include/stout/os/posix/stat.hpp 
1ab20e75fc18b336162b62e2f4f23b68f6685183 
  3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 

Diff: https://reviews.apache.org/r/55242/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[James Peach]

> On Jan. 13, 2017, 8:27 a.m., Jiang Yan Xu wrote:
> > 3rdparty/stout/tests/os_tests.cpp, line 751
> > 
> >
> > s/tree/subtree/?

This commend looks correct to me.


- James


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


On Jan. 13, 2017, 11:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> ---
> 
> (Updated Jan. 13, 2017, 11:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reimplement os::chown() to use fts(3) rather than sometimes spawning
> chown(1). This removes the use of the shell and the corresponding
> need to sanitize path arguments.  It also enables us to provide
> consistent handling of symbolic links for the recursive and
> non-recursive cases. We ensure that symlinks are never followed
> and that we always change the ownership of the link itself, not
> its referent.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/chown.hpp 
> c82e2e574019c5ee5f17ea105a6d225006388a45 
>   3rdparty/stout/include/stout/os/posix/stat.hpp 
> 1ab20e75fc18b336162b62e2f4f23b68f6685183 
>   3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 
> 
> Diff: https://reviews.apache.org/r/55242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[James Peach]

> On Jan. 13, 2017, 8:27 a.m., Jiang Yan Xu wrote:
> > 3rdparty/stout/tests/os_tests.cpp, line 751
> > 
> >
> > s/tree/subtree/?

This commend looks correct to me.


- James


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


On Jan. 13, 2017, 11:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> ---
> 
> (Updated Jan. 13, 2017, 11:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reimplement os::chown() to use fts(3) rather than sometimes spawning
> chown(1). This removes the use of the shell and the corresponding
> need to sanitize path arguments.  It also enables us to provide
> consistent handling of symbolic links for the recursive and
> non-recursive cases. We ensure that symlinks are never followed
> and that we always change the ownership of the link itself, not
> its referent.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/chown.hpp 
> c82e2e574019c5ee5f17ea105a6d225006388a45 
>   3rdparty/stout/include/stout/os/posix/stat.hpp 
> 1ab20e75fc18b336162b62e2f4f23b68f6685183 
>   3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 
> 
> Diff: https://reviews.apache.org/r/55242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55241: Stop using os::system to validate perf event names.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Jan. 13, 2017, 3:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55241/
> ---
> 
> (Updated Jan. 13, 2017, 3:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to validate perf event names.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 
> 
> Diff: https://reviews.apache.org/r/55241/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[James Peach]

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

(Updated Jan. 13, 2017, 11:29 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

To be consistent with Master::statusUpdateAcknowledgement(), validate
that the UUID in the StatusUpdate message is acceptable to the
UUID::fromBytes() parser.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 

Diff: https://reviews.apache.org/r/55509/diff/


Testing
---

Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.


Thanks,

James Peach

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Sorry missed a few minor things.


src/master/master.cpp (line 5848)


Sorry forgot to mention that 

s/uuid_/uuid/ (no need for it, the other instance in this file has a method 
argument `uuid` already)



src/master/master.cpp (line 5852)


s/":"/": "/


- Jiang Yan Xu


On Jan. 13, 2017, 3:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55509/
> ---
> 
> (Updated Jan. 13, 2017, 3:29 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6920
> https://issues.apache.org/jira/browse/MESOS-6920
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be consistent with Master::statusUpdateAcknowledgement(), validate
> that the UUID in the StatusUpdate message is acceptable to the
> UUID::fromBytes() parser.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
> 
> Diff: https://reviews.apache.org/r/55509/diff/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55509: Validate the StatusUpdate UUID in Master::statusUpdate.

[James Peach]

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

(Updated Jan. 13, 2017, 11:38 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

To be consistent with Master::statusUpdateAcknowledgement(), validate
that the UUID in the StatusUpdate message is acceptable to the
UUID::fromBytes() parser.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 

Diff: https://reviews.apache.org/r/55509/diff/


Testing
---

Make check (Fedora 25). Manual testing with a handmade StatusUpdate message.


Thanks,

James Peach

Re: Review Request 53803: Added new libprocess socket tests.

[Joseph Wu]

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


Ship it!




I can tweak the below comments before committing.


3rdparty/libprocess/src/tests/socket_tests.cpp (line 89)


Typo on `Parametrize`



3rdparty/libprocess/src/tests/socket_tests.cpp (lines 133 - 140)


To future proof this code, this macro should (unfortunately) be expanded 
into:
```
#ifdef USE_SSL_SOCKET
INSTANTIATE_TEST_CASE_P(
Encryption,
NetSocketTest,
::testing::Values(
string("SSL"),
string("Non-SSL")));
#else
INSTANTIATE_TEST_CASE_P(
Encryption,
NetSocketTest,
::testing::Values(
string("Non-SSL")));
#endif // USE_SSL_SOCKET
```

By the looks of it, this test will be ported to Windows, which means that 
MSVC will refuse to expand the #ifdef inside the test macro.



3rdparty/libprocess/src/tests/socket_tests.cpp (lines 147 - 148)


Let's move this to the upper part of the file.


- Joseph Wu


On Jan. 11, 2017, 4:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53803/
> ---
> 
> (Updated Jan. 11, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-6802
> https://issues.apache.org/jira/browse/MESOS-6802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds NetSocketTest.EOFBeforeRecv and
> NetSocketTest.EOFAfterRecv to verify that EOFs are
> reliably received whether or not there is a pending recv()
> request at the time the EOF is received.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/socket_tests.cpp 
> 44c3c9adc39702dd598aa0105088517df601bbda 
> 
> Diff: https://reviews.apache.org/r/53803/diff/
> 
> 
> Testing
> ---
> 
> Testing details are at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55343: Fixed SSL socket 'shutdown()'.

[Joseph Wu]

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


Ship it!




I can tweak this before committing.


3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 53 - 56)


I'd reword this a bit as a `NOTE` as ignoring the `how` parameter might be 
a significant detail.



3rdparty/libprocess/src/libevent_ssl_socket.cpp 


We should keep this comment around.


- Joseph Wu


On Jan. 13, 2017, 11:19 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55343/
> ---
> 
> (Updated Jan. 13, 2017, 11:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joseph Wu.
> 
> 
> Bugs: MESOS-6789
> https://issues.apache.org/jira/browse/MESOS-6789
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Recently, a change was made to the signature of
> `Socket::shutdown`, but the corresponding override in
> `LibeventSSLSocketImpl` was not updated, so that the
> implementation-specific method is no longer being
> executed. Further, the SSL socket's `shutdown` code
> did not actually shutdown the socket; rather, the
> shutdown was performed in the destructor.
> 
> This patch updates the function's signature to match
> that of the base class's method, adds the `override`
> specifier to the implemention's method declaration,
> and updates the function to properly shutdown the
> SSL socket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> 87966155aa21328db51796b2ae0a883054c00457 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 65da091155107d77bdf7b003609ab3770f80083a 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> b0319b2d3694f600190615ba6d29b95b1d8f2405 
> 
> Diff: https://reviews.apache.org/r/55343/diff/
> 
> 
> Testing
> ---
> 
> `3rdparty/libprocess/libprocess-tests 
> --gtest_filter="Encryption/NetSocketTest.EOFBeforeRecv/0:Encryption/NetSocketTest.EOFAfterRecv/0"
>  --gtest_repeat=-1 --gtest_break_on_failure`
> 
> NOTE: currently the above command exposes a file descriptor leak related to 
> the SSL socket.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55488: Slimmed down the size of the docker image from 2GB to roughly 650MB.

[Michael Park]

> On Jan. 13, 2017, 3:53 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy/Dockerfile, lines 69-77
> > 
> >
> > These packages are from the Mesos getting started guide, and I am not 
> > sure we would like to deviate here. It does save about 400MB in this layer 
> > (probably since we do not install the java packages), but we'll likely be 
> > perpetually out of sync as the Mesos cmake setup approaches the autotool 
> > setup's capabilities.

- `libev` is bundled.
- `libevent` is not, but we download it via `ExternalProject` for CMake.
- `libssl` is not supported by CMake, but I imagine we'll be taking the same 
approach as `libevent`. We can adjust if it changes.
- We don't build the Java stuff with CMake and frankly I don't think we care 
about it... so I dropped `maven` and `openjdk-8-jdk`.

Let me know if you still think I should add any of them back.


- Michael


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


On Jan. 13, 2017, 1:54 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55488/
> ---
> 
> (Updated Jan. 13, 2017, 1:54 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
> 
> Diff: https://reviews.apache.org/r/55488/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55489: Used CMake to generate the compilation database instead.

[Michael Park]

> On Jan. 13, 2017, 4 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 19
> > 
> >
> > Should we also set `pipefail`?

I didn't know about that. Sounds great.


> On Jan. 13, 2017, 4 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 34
> > 
> >
> > It seems something like this would still be useful for the cmake build 
> > so callers could pass extra configuration flags (e.g., of the form 
> > `-DENABLE_DEBUG=ON`).

I'll add this back.


> On Jan. 13, 2017, 4 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy/entrypoint.sh, line 27
> > 
> >
> > Left over debug tooling?

I use this a bunch of times below.


> On Jan. 13, 2017, 4 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy/entrypoint.sh, line 33
> > 
> >
> > I don't think the folks interested in cmake would ever see this comment 
> > here. We should create tickets and link them here.

Created MESOS-6924 and MESOS-6925


- Michael


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


On Jan. 13, 2017, 1:59 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55489/
> ---
> 
> (Updated Jan. 13, 2017, 1:59 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used CMake to generate the compilation database instead.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 
>   support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
>   support/mesos-tidy/entrypoint.sh 72872375f3e5ad19bc75949f9e3db14d6068f9b6 
> 
> Diff: https://reviews.apache.org/r/55489/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55521: Handle perf versions with more than 3 components.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55521]

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 Jan. 13, 2017, 8:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55521/
> ---
> 
> (Updated Jan. 13, 2017, 8:24 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Kapil Arya, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6552 and MESOS-6867
> https://issues.apache.org/jira/browse/MESOS-6552
> https://issues.apache.org/jira/browse/MESOS-6867
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle perf versions with more than 3 components.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp c7dfe0ac4965a450b25085715502f49bee972b5c 
>   src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 
>   src/tests/containerizer/perf_tests.cpp 
> e5ee3e71478e167c345e4a9a7c2c306d302b028b 
> 
> Diff: https://reviews.apache.org/r/55521/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [55238, 55239, 55240, 55241, 55242]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh 2>&1 | tee 
build_55242"]

Error:
bash: ./support/docker_build.sh: No such file or directory

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16720/console

- Mesos ReviewBot


On Jan. 13, 2017, 11:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> ---
> 
> (Updated Jan. 13, 2017, 11:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reimplement os::chown() to use fts(3) rather than sometimes spawning
> chown(1). This removes the use of the shell and the corresponding
> need to sanitize path arguments.  It also enables us to provide
> consistent handling of symbolic links for the recursive and
> non-recursive cases. We ensure that symlinks are never followed
> and that we always change the ownership of the link itself, not
> its referent.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/chown.hpp 
> c82e2e574019c5ee5f17ea105a6d225006388a45 
>   3rdparty/stout/include/stout/os/posix/stat.hpp 
> 1ab20e75fc18b336162b62e2f4f23b68f6685183 
>   3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 
> 
> Diff: https://reviews.apache.org/r/55242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>