Re: Review Request 36413: Removed the code of setting SCHED_IDLE policy for revocable containers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36410, 36411, 36412, 36413]

All tests passed.

- Mesos ReviewBot


On July 11, 2015, 12:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36413/
> ---
> 
> (Updated July 11, 2015, 12:09 a.m.)
> 
> 
> Review request for mesos, Ian Downes and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed the code of setting SCHED_IDLE policy for revocable containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 
> 
> Diff: https://reviews.apache.org/r/36413/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [34908, 34136, 34137]

Failed command: ./support/apply-review.sh -n -r 34137

Error:
 2015-07-11 00:17:12 URL:https://reviews.apache.org/r/34137/diff/raw/ 
[27315/27315] -> "34137.patch" [1]
error: patch failed: include/mesos/slave/isolator.hpp:30
error: include/mesos/slave/isolator.hpp: patch does not apply
error: patch failed: src/Makefile.am:525
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:548
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On July 11, 2015, 12:05 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> ---
> 
> (Updated July 11, 2015, 12:05 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved code from Mesos Containerizer to filesystem isolators
>  - filesystem/posix (symlinks, doesn't support container rootfs)
>  - filesystem/linux (bind mounts, does support container rootfs)
> 
> The filesystem/posix isolator will be automatically included if no 
> filesystem/ isolator is specified.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> 8eae258d81229e19f8c587f5e023b1df7deed025 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
> 
> Diff: https://reviews.apache.org/r/34135/diff/
> 
> 
> Testing
> ---
> 
> existing persistent volumes tests.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 36411: Used low cpu.shares for revocable containers.

[Jie Yu]

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

Review request for mesos, Ian Downes and Vinod Kone.


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


Repository: mesos


Description
---

Used low cpu.shares for revocable containers.


Diffs
-

  src/slave/containerizer/isolators/cgroups/constants.hpp 
e6df4a29e9af8076d6454740afa61fce04c3d06b 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp 
f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 36413: Removed the code of setting SCHED_IDLE policy for revocable containers.

[Jie Yu]

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

Review request for mesos, Ian Downes and Vinod Kone.


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


Repository: mesos


Description
---

Removed the code of setting SCHED_IDLE policy for revocable containers.


Diffs
-

  src/slave/containerizer/isolators/cgroups/cpushare.cpp 
f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 36410: Changed the MIN_CPU_SHARES to match the kernel constant.

[Jie Yu]

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

Review request for mesos, Ian Downes and Vinod Kone.


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


Repository: mesos


Description
---

Changed the MIN_CPU_SHARES to match the kernel constant.


Diffs
-

  src/slave/containerizer/isolators/cgroups/constants.hpp 
e6df4a29e9af8076d6454740afa61fce04c3d06b 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 36412: Adjusted the revocable cpu isolator test.

[Jie Yu]

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

Review request for mesos, Ian Downes and Vinod Kone.


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


Repository: mesos


Description
---

Adjusted the revocable cpu isolator test.


Diffs
-

  src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 34137: Add support for container image provisioners.

[Ian Downes]

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

(Updated July 10, 2015, 5:05 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Addressed comments.


Repository: mesos


Description
---

The MesosContainerizer can optionally provision a root filesystem for the 
containers.


Diffs
-

  include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 
  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/mesos/containerizer.hpp 
3ac2387eabded61c897a5016655aae10cd1bca91 
  src/slave/containerizer/mesos/containerizer.cpp 
8c102fb7d1f79ee768cb06de3a976ea12f958712 
  src/slave/containerizer/provisioner.hpp PRE-CREATION 
  src/slave/containerizer/provisioner.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
  src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
  src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
  src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 
  src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec 
  src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.

[Ian Downes]

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

(Updated July 10, 2015, 5:05 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Repository: mesos


Description
---

Moved code from Mesos Containerizer to filesystem isolators
 - filesystem/posix (symlinks, doesn't support container rootfs)
 - filesystem/linux (bind mounts, does support container rootfs)

The filesystem/posix isolator will be automatically included if no filesystem/ 
isolator is specified.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION 
  src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION 
  src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION 
  src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION 
  src/slave/containerizer/linux_launcher.cpp 
8eae258d81229e19f8c587f5e023b1df7deed025 
  src/slave/containerizer/mesos/containerizer.cpp 
8c102fb7d1f79ee768cb06de3a976ea12f958712 

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


Testing
---

existing persistent volumes tests.


Thanks,

Ian Downes

Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.

[Ian Downes]

> On June 25, 2015, 12:47 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 406-407
> > 
> >
> > Do you need to umount persistent volumes as well?

As the comment states, it "notably this includes persistent volume mounts" :-)


> On June 25, 2015, 12:47 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 158-166
> > 
> >
> > Hum... why this logic is under 'if 
> > (ModuleManager::contains(type)' ?

Good catch! No idea why it was there in the first place and why I perpetuated 
the erroneous code.


- Ian


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


On June 22, 2015, 9:41 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> ---
> 
> (Updated June 22, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved code from Mesos Containerizer to filesystem isolators
>  - filesystem/posix (symlinks, doesn't support container rootfs)
>  - filesystem/linux (bind mounts, does support container rootfs)
> 
> The filesystem/posix isolator will be automatically included if no 
> filesystem/ isolator is specified.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> 8eae258d81229e19f8c587f5e023b1df7deed025 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
> 
> Diff: https://reviews.apache.org/r/34135/diff/
> 
> 
> Testing
> ---
> 
> existing persistent volumes tests.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Timothy Chen]

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



src/slave/containerizer/docker.cpp (line 1325)


We also have a unit test in docker_containerizer_tests testing the usage 
call, can you see if it makes sense to add something to test the rss data as 
well?


- Timothy Chen


On July 10, 2015, 11:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 10, 2015, 11:03 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36106, 36326]

All tests passed.

- Mesos ReviewBot


On July 10, 2015, 11:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 10, 2015, 11:03 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 34908: Rename --docker_sandbox_directory flag for general use.

[Timothy Chen]

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

Ship it!


Ship It!


docs/configuration.md (line 933)


Can we go through a depcreation cycle for this flag? I'm not 100% sure 
who's using this but we should give users a warning and remove it at the 
following version.


- Timothy Chen


On June 22, 2015, 4:55 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34908/
> ---
> 
> (Updated June 22, 2015, 4:55 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename --docker_sandbox_directory flag for general use.
> 
> This will require users to update configuration scripts etc if they specify 
> the old flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677 
>   src/slave/containerizer/docker.cpp 00db9811824995fe19a6143aa2bcba3733a93147 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
>   src/tests/docker_containerizer_tests.cpp 
> 3a983c6813dab6fa03ccb2c87e1ea71866766d6e 
> 
> Diff: https://reviews.apache.org/r/34908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36185: Create pre-launch hook before a docker container launches in slave.

[Timothy Chen]

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



src/tests/docker_containerizer_tests.cpp (line 3114)


What you suggested sounds fine.
I think we need to comment on the top of the test what this tests and what 
is it expecting to see. It wasn't too obvious when I read this.


- Timothy Chen


On July 8, 2015, 5:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36185/
> ---
> 
> (Updated July 8, 2015, 5:06 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2588
> https://issues.apache.org/jira/browse/MESOS-2588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Create pre-launch hook before a docker container launches in slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f 
>   src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab 
>   src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc 
>   src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf 
>   src/slave/containerizer/docker.cpp cfb60177fe48ec0eeab12ff392c6c9f89634b92f 
>   src/tests/docker_containerizer_tests.cpp 
> a3da786c1b733e9dd4abf1d02d5dae051393d921 
> 
> Diff: https://reviews.apache.org/r/36185/diff/
> 
> 
> Testing
> ---
> 
> # Add a new unit test 
> "DockerContainerizerTest.ROOT_DOCKER_VerifySlavePreLaunchDockerHook"
> make check -j8 GTEST_FILTER=-"*"
> sudo ./bin/mesos-tests.sh --verbose 
> --gtest_filter=DockerContainerizerTest.ROOT_DOCKER_VerifySlavePreLaunchDockerHook
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 36402: Adding 'Accept' header in request

[Anand Mazumdar]

> On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 216
> > 
> >
> > I did not review the entire patch but I stopped at this. Seems like I 
> > am missing something here. 
> > 
> > In my understanding, you can't use the same method for parsing both the 
> > "Accept", "Accept-Encoding" as the rules for both of them are entirely 
> > different ! :)
> > 
> > Let's take an example from the RFC : 
> > http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
> > 
> > So the accept header also understands media-range i.e. you can specify 
> > "*/*" or "text/*" et al meaning all media types can be accepted or in the 
> > second case all media types of text/something can only be accepted and so 
> > on.
> > 
> > There are a couple of other things like accept-param and 
> > accept-extension that also need to be handled.
> > 
> > I assume that your motive for this change was to add a validation 
> > operation for "Accept" similar to "Accept-Encoding" header that validates 
> > if the header values are well-formed and should be accepted ? If that is 
> > the case, you would need to write a separate method/logic and not just use 
> > the existing acceptEncoding method.
> > 
> > Long story short, we need two methods:
> > 1. Validate if the "Accept" header is well formed. ( the one you 
> > already built minus the mentioned caveats above )
> > 
> > Also , it would be good to add a second one:
> > 2. Given all the accept types mentioned in the "Accept" header , and 
> > the accept types we support ,is it possible for us to send a response back 
> > ? If not send a 415 back.
> > 
> > What do you think ?
> 
> Isabel Jimenez wrote:
> The AcceptHeader method groups validation that's common to both 'Accept' 
> and 'Accept-Encoding'. 
> The logic was already there I just moved it so we can use it for both, 
> and if needed and more things to each one separately. 
> 
> I plan to add 'accept-param' and 'accept-extension' support in a 
> different patch. 
> 
> I also added a TODO to consider handling all this by returning Response, 
> what do you think, should we make that change now?

Unfortunately, This does not make much sense to me. Let's take an example from 
your test-case.

requests[2].headers["Accept"] = "foo, test;q=0.0";

This is not a "VALID" accept header at all if you see its grammer carefully. 
The only thing that "Accept" and "Accept-Encoding" have in common is the 
q-value syntax and how you specify them on one line i.e. delimited by "," and 
";". :)

An "Accept" header must have a "type"/"subtype" or a "type/*".

Makes sense ? ( I am re-opening the issue )


- Anand


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


On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> ---
> 
> (Updated July 10, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 72b6d27 
>   3rdparty/libprocess/src/encoder.hpp c5ff761 
>   3rdparty/libprocess/src/http.cpp d168579 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
>   3rdparty/libprocess/src/tests/http_tests.cpp 01f243c 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Jojy Varghese]

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

(Updated July 10, 2015, 11:03 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

review comments @tim


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


Repository: mesos


Description (updated)
---

Adding cgroups cpustat and memory statistics as preferred way to get usage
statistics for containerizers.

Jira: MESOS-2951


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
  src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Jojy Varghese]

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

(Updated July 10, 2015, 10:47 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

review comments @tim


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


Repository: mesos


Description
---

WIP

Adding cgroups cpustat and memory statistics as preferred way to get usage
statistics for containerizers.

Jira: MESOS-2951


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
  src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Jojy Varghese]

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

(Updated July 10, 2015, 10:46 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy 
Chen.


Changes
---

code review @benm


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


Repository: mesos


Description
---

cgroups implementation does not have a cpuacct subsystem implementation as of
today. Adding the implementation for stat function.

Changes:
  - added Stats class to encapsulate cpuacct.stat data
  - added implementation for cpuacct::stats
  - added unit tests

Jira: MESOS-2961


Diffs (updated)
-

  src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
  src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
  src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 36402: Adding 'Accept' header in request

[Marco Massenzio]

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



3rdparty/libprocess/src/http.cpp (lines 164 - 183)


These appear to me to be valuable information for a caller too - please 
move those "up" to be the method's Doxy (javadoc) in addition to writing up the 
method's description.

Please make sure to make them consistent with our style for comments.



3rdparty/libprocess/src/http.cpp (line 190)


ditto



3rdparty/libprocess/src/tests/http_tests.cpp (lines 676 - 689)


```
vector bogusHeaders = { "test;q=0.0",
"foo",
"foo, test;q=0.0",
"*, test;q=0.0",
"*;q=0.0, foo",
"\n foo",
"foo,\ttest;q=0.0"};

http::Request request;
for (auto accept : bogusHeaders) {
  request.headers["Accept"] = accept;
  EXPECT_FALSE(request.acceptsMediaType("test").get())
}
```
Let's please make the most out of our newly-acquired C++11 abilities :)



3rdparty/libprocess/src/tests/http_tests.cpp (line 695)


same here


- Marco Massenzio


On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> ---
> 
> (Updated July 10, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 72b6d27 
>   3rdparty/libprocess/src/encoder.hpp c5ff761 
>   3rdparty/libprocess/src/http.cpp d168579 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
>   3rdparty/libprocess/src/tests/http_tests.cpp 01f243c 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36402: Adding 'Accept' header in request

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36402]

All tests passed.

- Mesos ReviewBot


On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> ---
> 
> (Updated July 10, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 72b6d27 
>   3rdparty/libprocess/src/encoder.hpp c5ff761 
>   3rdparty/libprocess/src/http.cpp d168579 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
>   3rdparty/libprocess/src/tests/http_tests.cpp 01f243c 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36402: Adding 'Accept' header in request

[Isabel Jimenez]

> On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 216
> > 
> >
> > I did not review the entire patch but I stopped at this. Seems like I 
> > am missing something here. 
> > 
> > In my understanding, you can't use the same method for parsing both the 
> > "Accept", "Accept-Encoding" as the rules for both of them are entirely 
> > different ! :)
> > 
> > Let's take an example from the RFC : 
> > http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
> > 
> > So the accept header also understands media-range i.e. you can specify 
> > "*/*" or "text/*" et al meaning all media types can be accepted or in the 
> > second case all media types of text/something can only be accepted and so 
> > on.
> > 
> > There are a couple of other things like accept-param and 
> > accept-extension that also need to be handled.
> > 
> > I assume that your motive for this change was to add a validation 
> > operation for "Accept" similar to "Accept-Encoding" header that validates 
> > if the header values are well-formed and should be accepted ? If that is 
> > the case, you would need to write a separate method/logic and not just use 
> > the existing acceptEncoding method.
> > 
> > Long story short, we need two methods:
> > 1. Validate if the "Accept" header is well formed. ( the one you 
> > already built minus the mentioned caveats above )
> > 
> > Also , it would be good to add a second one:
> > 2. Given all the accept types mentioned in the "Accept" header , and 
> > the accept types we support ,is it possible for us to send a response back 
> > ? If not send a 415 back.
> > 
> > What do you think ?

The AcceptHeader method groups validation that's common to both 'Accept' and 
'Accept-Encoding'. 
The logic was already there I just moved it so we can use it for both, and if 
needed and more things to each one separately. 

I plan to add 'accept-param' and 'accept-extension' support in a different 
patch. 

I also added a TODO to consider handling all this by returning Response, what 
do you think, should we make that change now?


- Isabel


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


On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> ---
> 
> (Updated July 10, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 72b6d27 
>   3rdparty/libprocess/src/encoder.hpp c5ff761 
>   3rdparty/libprocess/src/http.cpp d168579 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
>   3rdparty/libprocess/src/tests/http_tests.cpp 01f243c 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36404: Added support for peek() to process::io

[Artem Harutyunyan]

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

(Updated July 10, 2015, 3:20 p.m.)


Review request for mesos, Joris Van Remoortere and Joseph Wu.


Repository: mesos


Description
---

JIRA: https://issues.apache.org/jira/browse/MESOS-2964


Diffs (updated)
-

  3rdparty/libprocess/include/process/io.hpp 
245716353ad5ffa8d705fc5e826addfa6a3594dc 
  3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 
  3rdparty/libprocess/src/tests/io_tests.cpp 
c642bab9e2845668767ad237985cb9ce1109 

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


Testing
---

- Added a test case for process::io::peek
- make check


Thanks,

Artem Harutyunyan

Re: Review Request 36402: Adding 'Accept' header in request

[Anand Mazumdar]

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



3rdparty/libprocess/src/http.cpp (line 206)


I did not review the entire patch but I stopped at this. Seems like I am 
missing something here. 

In my understanding, you can't use the same method for parsing both the 
"Accept", "Accept-Encoding" as the rules for both of them are entirely 
different ! :)

Let's take an example from the RFC : 
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

So the accept header also understands media-range i.e. you can specify 
"*/*" or "text/*" et al meaning all media types can be accepted or in the 
second case all media types of text/something can only be accepted and so on.

There are a couple of other things like accept-param and accept-extension 
that also need to be handled.

I assume that your motive for this change was to add a validation operation 
for "Accept" similar to "Accept-Encoding" header that validates if the header 
values are well-formed and should be accepted ? If that is the case, you would 
need to write a separate method/logic and not just use the existing 
acceptEncoding method.

Long story short, we need two methods:
1. Validate if the "Accept" header is well formed. ( the one you already 
built minus the mentioned caveats above )

Also , it would be good to add a second one:
2. Given all the accept types mentioned in the "Accept" header , and the 
accept types we support ,is it possible for us to send a response back ? If not 
send a 415 back.

What do you think ?


- Anand Mazumdar


On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> ---
> 
> (Updated July 10, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 72b6d27 
>   3rdparty/libprocess/src/encoder.hpp c5ff761 
>   3rdparty/libprocess/src/http.cpp d168579 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
>   3rdparty/libprocess/src/tests/http_tests.cpp 01f243c 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Jojy Varghese]

> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1266
> > 
> >
> > I think you reordered things here that used to be in _usage.
> > 
> > We used to first check the containers map, then check is it destroying, 
> > that look at the pid and finally passing that on.
> > 
> > Here the order is now pid check, and some reason skip the map check to 
> > get it but then checking it later on.
> > 
> > I think we should go back to the exact same order as before unless you 
> > have reasons to do this change.
> 
> Jojy Varghese wrote:
> The thought was as follows:
> - We called inspect to get the pid. So first we check for a pid.  Then we 
> set the pid for the container (if the container exists). Then we call 
> "collectUsage" to collect stats.
> - The check for DESTROYING is already being centralized in the 
> collectUsage lambda.

wanted to point out that we are not skipping the check for contanier at all. We 
pick the pid from the Docker::Container object being passed in by the "then" 
continuation. We still check the existence of container in the collection 
(member variable collection_).


- Jojy


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


On July 9, 2015, 8:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 9, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

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



src/linux/perf.cpp (line 468)


Get version number from 'perf --version' not os::release.



src/linux/perf.cpp (line 623)


We should get the version number from 'perf --version' not os::release in 
case a mismatched perf is installed.


- Paul Brett


On July 10, 2015, 8:48 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> ---
> 
> (Updated July 10, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output 
> versions depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Jojy Varghese]

> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1266
> > 
> >
> > I think you reordered things here that used to be in _usage.
> > 
> > We used to first check the containers map, then check is it destroying, 
> > that look at the pid and finally passing that on.
> > 
> > Here the order is now pid check, and some reason skip the map check to 
> > get it but then checking it later on.
> > 
> > I think we should go back to the exact same order as before unless you 
> > have reasons to do this change.

The thought was as follows:
- We called inspect to get the pid. So first we check for a pid.  Then we set 
the pid for the container (if the container exists). Then we call 
"collectUsage" to collect stats.
- The check for DESTROYING is already being centralized in the collectUsage 
lambda.


> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1272
> > 
> >
> > If you like to a leave a comment why we called this function we should 
> > probably comment at the place we call it not in the function (before 
> > docker->inspect).
> > 
> > also s/didnt/didn't/g

Wanted to emphasize the logic of why we need to update the pid there (and not 
say in collectUSage).


> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1342
> > 
> >
> > This seems too verbose being LOG(INFO).
> > And what value do you think we have here logging this?

I can change it to LOG(DEBUG). Wanted to log there to help us debug.


- Jojy


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


On July 9, 2015, 8:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 9, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35927: Added TaskStatus::Reason to Termination Message.

[Jie Yu]

> On June 29, 2015, 7:40 p.m., Jie Yu wrote:
> > Thanks Joerg! See my detailed comments. I suggest you take a look at the 
> > discussion in https://reviews.apache.org/r/34720/ first.

Hi Joerg, I am wondering if you are still working on this ticket? Some of our 
internal framework developers noticed that Mesos sends a TASK_FAILED with 
REASON_MEMORY_LIMIT even if he just specified the wrong executor command (thus 
causing executor registration timeout). This is very misleading and we want to 
fix that asap.


- Jie


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


On June 29, 2015, 7:22 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35927/
> ---
> 
> (Updated June 29, 2015, 7:22 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-2035
> https://issues.apache.org/jira/browse/MESOS-2035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [WIP] 
>  -need to add documentation
>  -need to add test 
>  -not all potential paths are covered. It is not clear whether isolator sees 
> event before the kill, see first comment in CgroupsMemIsolatorProcess::oom in 
> mem.cpp.
> 
> 
> Diffs
> -
> 
>   docs/external-containerizer.md 96932189b74dce1ae28e0bd73d5543d1afaffb0b 
>   include/mesos/containerizer/containerizer.proto 
> f16ccc89f83da28c413ccfa0687a06b7515a605c 
>   include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19 
>   include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> b0e343fdc7088b2895d5dc8bb416dbcbf241cae5 
>   src/slave/containerizer/isolators/posix/disk.cpp 
> b2f995cba36b1399db48af1de49d76c607f80abd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 313e9b74d3a0157609226041246575d2c4a503f8 
>   src/slave/slave.cpp b8591116eadcd68b8db2a629fbcf793e6b394f14 
>   src/tests/docker_containerizer_tests.cpp 
> 3a983c6813dab6fa03ccb2c87e1ea71866766d6e 
> 
> Diff: https://reviews.apache.org/r/35927/diff/
> 
> 
> Testing
> ---
> 
> make check (including docker tests)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.

[Ian Downes]

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



src/tests/perf_tests.cpp (line 52)


split args onto separate lines



src/tests/perf_tests.cpp (line 62)


lines? it's 1 or more, how about just calling input?

is debug() a better name for this function?



src/tests/perf_tests.cpp (lines 65 - 69)


use a ternary if here?
```cpp
s << (version.isError() ? "Error:" + version.error()
: version.get());
```



src/tests/perf_tests.cpp (line 104)


I don't think we use this, favoring 
```cpp
foreach (const tuple&, input)
```

Is Version hashable? If so, iterating over a Version -> input would be much 
cleaner
```cpp
foreachpair(const Version& version, const string& input, inputs)
```

Or, just use the string version and parse the string version inside the 
loop?
```cpp
hashmap input {
  "2.6.39", "123,cycles\n0.123,task-clock"
}
```



src/tests/perf_tests.cpp (line 157)


Do you need to keep all the logging of the context  on failed expectations 
after the test has been correctly written? It clutters the code...



src/tests/perf_tests.cpp (line 191)


ditto



src/tests/perf_tests.cpp (line 226)


ditto


- Ian Downes


On July 10, 2015, 1:52 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36380/
> ---
> 
> (Updated July 10, 2015, 1:52 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test cases for performance monitor support of multiple output versions 
> depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 
> 
> Diff: https://reviews.apache.org/r/36380/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36378, 36380]

All tests passed.

- Mesos ReviewBot


On July 10, 2015, 8:52 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36380/
> ---
> 
> (Updated July 10, 2015, 8:52 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test cases for performance monitor support of multiple output versions 
> depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 
> 
> Diff: https://reviews.apache.org/r/36380/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

> On July 10, 2015, 5:25 p.m., Ian Downes wrote:
> > src/linux/perf.cpp, line 482
> > 
> >
> > Suggest moving the TODO to here.

I'd like to keep it with the definition since usage is in multiple locations in 
the file.


- Paul


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


On July 10, 2015, 8:48 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> ---
> 
> (Updated July 10, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output 
> versions depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Timothy Chen]

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



src/linux/cgroups.hpp (line 438)


benm: Remove 1 space.



src/linux/cgroups.cpp (line 2003)


benm: s/std::string/string/g



src/linux/cgroups.cpp (line 2004)


ditto



src/linux/cgroups.cpp (line 2022)


benm: s/Error getting/Failed to get/g

Also we should use ErrnoError as well.



src/linux/cgroups.cpp (line 2030)


benm: All the error messages in this file follows the "Failed to..." 
instead of "Error..." message, we should be consistent.



src/tests/cgroups_tests.cpp (line 62)


benm: Remove alias



src/tests/cgroups_tests.cpp (line 1199)


benm: Remove storing result and just 
ASSERT_SOME(cgroups::cpuacct::stat(hierarchy, TEST_CGROUPS_ROOT));



src/tests/cgroups_tests.cpp (line 1203)


benm: AWAIT_READY(cgroups::destroy(hierarchy, TEST_CGROUPS_ROOT));


- Timothy Chen


On July 10, 2015, 8:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 10, 2015, 8:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2961
> https://issues.apache.org/jira/browse/MESOS-2961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> cgroups implementation does not have a cpuacct subsystem implementation as of
> today. Adding the implementation for stat function.
> 
> Changes:
>   - added Stats class to encapsulate cpuacct.stat data
>   - added implementation for cpuacct::stats
>   - added unit tests
> 
> Jira: MESOS-2961
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
>   src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
>   src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 
> 
> Diff: https://reviews.apache.org/r/36106/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 36402: Adding 'Accept' header in request

[Isabel Jimenez]

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

Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
Vinod Kone.


Repository: mesos-incubating


Description
---

Adding a method for Accept header in request + refactor of Accept-Encoding


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 72b6d27 
  3rdparty/libprocess/src/encoder.hpp c5ff761 
  3rdparty/libprocess/src/http.cpp d168579 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
  3rdparty/libprocess/src/tests/http_tests.cpp 01f243c 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.

[Paul Brett]

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

(Updated July 10, 2015, 8:52 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


Changes
---

Corrected kernel version number for introduction of change.  Added error 
diagnostics.


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


Repository: mesos


Description
---

Test cases for performance monitor support of multiple output versions 
depending on kernel version.


Diffs (updated)
-

  src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Timothy Chen]

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



src/slave/containerizer/docker.cpp (line 1216)


let's spell out cgStats to cgroupStats



src/slave/containerizer/docker.cpp (line 1219)


"Error getting cgroups statistics for container '" + stringify(containerId) 
+ "': " + cgroupStats.error()"

And why the extra LOG(WARNING) here?



src/slave/containerizer/docker.cpp (line 1254)


I think you reordered things here that used to be in _usage.

We used to first check the containers map, then check is it destroying, 
that look at the pid and finally passing that on.

Here the order is now pid check, and some reason skip the map check to get 
it but then checking it later on.

I think we should go back to the exact same order as before unless you have 
reasons to do this change.



src/slave/containerizer/docker.cpp (line 1260)


If you like to a leave a comment why we called this function we should 
probably comment at the place we call it not in the function (before 
docker->inspect).

also s/didnt/didn't/g



src/slave/containerizer/docker.cpp (line 1302)


Although it seems obvious from the right hand side you're getting the stat 
of cgroup, it wasn't obvious what type are you getting back, and more so about 
it's actually a Try
If you look at our style guide we only try to use auto in places the right 
hand side completely capture the type information. I think we should spell out 
the type here.



src/slave/containerizer/docker.cpp (line 1327)


This seems too verbose being LOG(INFO).
And what value do you think we have here logging this?


- Timothy Chen


On July 9, 2015, 8:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 9, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

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

(Updated July 10, 2015, 8:48 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


Changes
---

Fixed bug handling multiple counters in cgroups.


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


Repository: mesos


Description
---

Refactor Linux Performance monitor to handle changing 'perf stat' output 
versions depending on kernel version.


Diffs (updated)
-

  src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Jojy Varghese]

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

(Updated July 10, 2015, 8:47 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy 
Chen.


Changes
---

review comments @benm


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


Repository: mesos


Description
---

cgroups implementation does not have a cpuacct subsystem implementation as of
today. Adding the implementation for stat function.

Changes:
  - added Stats class to encapsulate cpuacct.stat data
  - added implementation for cpuacct::stats
  - added unit tests

Jira: MESOS-2961


Diffs (updated)
-

  src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
  src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
  src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 34140: AppC image store

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/store.hpp (lines 22 - 33)


Reorder includes.











src/slave/containerizer/provisioners/appc/store.hpp (lines 68 - 69)


I am sure you'll remove these but just putting on reminder here.



src/slave/containerizer/provisioners/appc/store.hpp (lines 70 - 71)


Two empty lines used here. Kill one.



src/slave/containerizer/provisioners/appc/store.hpp (line 95)


Move to the source file?



src/slave/containerizer/provisioners/appc/store.cpp (line 103)


Suggestion:

Somewhere in Store class if we draw a directory layout everything would be 
a lot clearer.



src/slave/containerizer/provisioners/appc/store.cpp (line 148)


The code above captures the stage via `[=]()` so it's inconsistent here. 
Should be fine to keep using `[=]()`.



src/slave/containerizer/provisioners/appc/store.cpp (lines 154 - 170)


TODO: This looks a bit hacky but that's what it takes to use the fetcher. 
That's some refactoring for the future.



src/slave/containerizer/provisioners/appc/store.cpp (lines 188 - 189)


4 space indentation.



src/slave/containerizer/provisioners/appc/store.cpp (line 189)


Make "image.ci" a const variable.



src/slave/containerizer/provisioners/appc/store.cpp (line 243)


4 space indentation.



src/slave/containerizer/provisioners/appc/store.cpp (line 247)


Indentation off by one space.



src/slave/containerizer/provisioners/appc/store.cpp (line 268)


Just would like to mention the possibility of using `pigz` which speeds 
things up quite a bit but of course we need to check its availability.



src/slave/containerizer/provisioners/appc/store.cpp (line 274)


Why different here? The other two formats have output being 'image'.

FWIW I think image.aci.out looks clearer that it's a temp.



src/slave/containerizer/provisioners/appc/store.cpp (line 276)


The availability of this binary is rearer than the previous two. And "xz 
not found" is treated the same as "file not compressed". 

Maybe at least some logging to distinguish the two  cases. I understand 
that eventually the failure will occur in the untar step but it would be nice 
to see who the culprit is. (Maybe don't redirect stderr to /dev/null)?



src/slave/containerizer/provisioners/appc/store.cpp (line 349)


Shift indentation left by one space.



src/slave/containerizer/provisioners/appc/store.cpp (line 392)


"staging/XX to storage/hash"?



src/slave/containerizer/provisioners/appc/store.cpp (line 397)






src/slave/containerizer/provisioners/appc/store.cpp (line 408)


What does this mean? TODO: rm store?

We do need to remove the corrupted store to avoid accumulating too much 
space and at least avoid confusing.

So how about: verify the image before renaming and do not move it into 
storage if it fails. The failed image is not immediately removed so the 
operator can log into the box to inspect it in the staging directoy but we know:
1) Images in the staging directory are going to be cleaned up by slave 
restart.
2) Images in the storage directory are all valid.



src/slave/containerizer/provisioners/appc/store.cpp (line 412)


AppcImageManifest belongs to a later review /r/34142/.

Maybe the manifest protobuf and its validation can be pulled out into a 
separate review that bears no depednency on store, discovery, etc and can be 
placed lower in the review depedency chain/tree?



src/slave/containerizer/provisioners/appc/store.cpp (line 417)


Why have "sha512-" as id prefix but not on the path? It would be nice to 
have them be consistent.

If id with sha512- prefix is clear

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

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

(Updated July 10, 2015, 6:16 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Refactor Linux Performance monitor to handle changing 'perf stat' output 
versions depending on kernel version.


Diffs (updated)
-

  src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

> On July 10, 2015, 5:25 p.m., Ian Downes wrote:
> > src/linux/perf.cpp, lines 469-474
> > 
> >
> > We should document this behavior at os::release() and move this there 
> > as an alternative.
> > 
> > If it's kept in here then make static?
> > 
> > Argument should be named 'version', not 'v'.
> > 
> > Does it need to have such a long function name, particularly if it's 
> > local?
> > 
> > Version canonicalize(const Version& version).

MESOS-3029 raised to move function to stout, but the solution is not that 
obvious since we could (1) modify os::release to return the canonical version 
but then it would not match uname which would be an issue if you used it to try 
and find something like a kernel module or (2) provide a separate function 
which returns canonical release but then there is more potential for using the 
wrong one or (3) provide version with a custom sort order so that the returned 
string is as reported by uname but Version(3.0.0) sorts as less that 
Version(2.6.41).


- Paul


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


On July 10, 2015, 5:33 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> ---
> 
> (Updated July 10, 2015, 5:33 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output 
> versions depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

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

(Updated July 10, 2015, 5:33 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


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


Repository: mesos


Description
---

Refactor Linux Performance monitor to handle changing 'perf stat' output 
versions depending on kernel version.


Diffs (updated)
-

  src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

> On July 10, 2015, 3:11 a.m., Chi Zhang wrote:
> > src/linux/perf.cpp, line 449
> > 
> >
> > Using Try as a function parameter feels a bit non-idiomatic to me, 
> > though I agree it's less code this way.

In other places I would agree with you, but in this case we should be able to 
test for os::release returning an error so the Try makes sense.


> On July 10, 2015, 3:11 a.m., Chi Zhang wrote:
> > src/linux/perf.cpp, lines 476-483
> > 
> >
> > I think using a hashmap as an intermediate type is more flexisible.
> > 
> > Right now, the operation after 'extract' is the same for all versions, 
> > but we can make it version-dependent too in the future to support e.g. 
> > unit, running time, etc.

Disagree.  The Sample struct is local to this file, so it is very easy to 
change.  Using a hashmap introduces the possibility for runtime errors which 
are caught at compile time this way.


> On July 10, 2015, 3:11 a.m., Chi Zhang wrote:
> > src/linux/perf.cpp, lines 462-474
> > 
> >
> > This is great abstraction; I agree we should put it into stout. (maybe 
> > even have os::release just return canonicalized linux version?)

I will raise this as a separate ticket. Making os::release return the 
canonicalized linux version introduces the potential confusion that uname would 
no longer match os::release.


- Paul


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


On July 9, 2015, 11:08 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> ---
> 
> (Updated July 9, 2015, 11:08 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output 
> versions depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Ian Downes]

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



src/linux/perf.cpp (line 449)


Why a Try<> argument and then a CHECK()? The caller of os::release() should 
check the return so either check it in supported() or query and check it in 
_supported().

Also, should be const Version&.



src/linux/perf.cpp (lines 468 - 473)


We should document this behavior at os::release() and move this there as an 
alternative.

If it's kept in here then make static?

Argument should be named 'version', not 'v'.

Does it need to have such a long function name, particularly if it's local?

Version canonicalize(const Version& version).



src/linux/perf.cpp (line 481)


Suggest moving the TODO to here.



src/linux/perf.cpp (lines 484 - 485)


There's no map, it returns a single Sample?



src/linux/perf.cpp (lines 486 - 488)


This fits on a single line.



src/linux/perf.cpp (lines 490 - 492)


newlines, please ;-)



src/linux/perf.cpp (line 492)


Can you provide links to the documentation/commits that introduced these 
changes?



src/linux/perf.cpp (line 508)


Check how patch versions are handled is correct, e.g., this would match 
3.12.1 (if it existed) when I think the change was in 3.13.0 so it should be >= 
3.13.0



src/linux/perf.cpp (line 524)


newline



src/linux/perf.cpp (line 532)


const T&



src/linux/perf.cpp (line 536)


newline


- Ian Downes


On July 9, 2015, 4:08 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> ---
> 
> (Updated July 9, 2015, 4:08 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output 
> versions depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36197: Documented "how to become a committer".

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36197]

All tests passed.

- Mesos ReviewBot


On July 10, 2015, 9:14 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36197/
> ---
> 
> (Updated July 10, 2015, 9:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1825
> https://issues.apache.org/jira/browse/MESOS-1825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new document "committer-candidate-checklist.md" and wrote
> a paragraph about the path to committership in "committers.md".
> 
> 
> Diffs
> -
> 
>   docs/committer-candidate-checklist.md PRE-CREATION 
>   docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 
> 
> Diff: https://reviews.apache.org/r/36197/diff/
> 
> 
> Testing
> ---
> 
> The rendered files can be viewed here:
> 
> https://gist.github.com/bernd-mesos/00de63ae13efec4331be
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Alexander Rukletsov]

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



src/common/http.hpp (lines 52 - 53)


Let's convert them to static functions as per 
https://issues.apache.org/jira/browse/MESOS-1023. E.g. see 
https://reviews.apache.org/r/30841/.


- Alexander Rukletsov


On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 10:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco 
> Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-2860
> https://issues.apache.org/jira/browse/MESOS-2860
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding constants used commonly through the different HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp bbd063d 
>   src/common/http.cpp 73a4de1 
> 
> Diff: https://reviews.apache.org/r/36360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36197: Documented "how to become a committer".

[Bernd Mathiske]

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

(Updated July 10, 2015, 2:14 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.


Changes
---

Removed "unanimous" from the sentence about PMC election.


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


Repository: mesos


Description
---

Added new document "committer-candidate-checklist.md" and wrote
a paragraph about the path to committership in "committers.md".


Diffs (updated)
-

  docs/committer-candidate-checklist.md PRE-CREATION 
  docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 

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


Testing
---

The rendered files can be viewed here:

https://gist.github.com/bernd-mesos/00de63ae13efec4331be


Thanks,

Bernd Mathiske

Re: Review Request 36389: [WIP] Enable remote execution of arbitrary command.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36389]

All tests passed.

- Mesos ReviewBot


On July 10, 2015, 7:59 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> ---
> 
> (Updated July 10, 2015, 7:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Cody Maloney.
> 
> 
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2830
> 
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
> 
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
> 
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
> 
> For more details, see the design doc at:
> https://goo.gl/4npTMU
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a 
>   src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 
> 
> Diff: https://reviews.apache.org/r/36389/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> lots of manual testing (using Postman, REST client)
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 36389: [WIP] Enable remote execution of arbitrary command.

[Marco Massenzio]

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

(Updated July 10, 2015, 7:59 a.m.)


Review request for mesos, Benjamin Hindman and Cody Maloney.


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


Repository: mesos


Description
---

Jira: MESOS-2830

Under certain (maintenance) circumnstance, it may be necessary
(or desirable) to execute arbitrary operator's commands on the
slave (the entire fleet or a subset thereof) bypassing the Mesos
Task execution mechanism; this may typically be necessary for
maintenance and/or emergency actions.

This patch adds an HTTP endpoint (/execute) which accepts a
JSON-encoded CommandInfo structure and executes the given
command (with optional arguments).

The output of the command (along with potentially any stderr
messages) is returned JSON-encoded in the Response.

For more details, see the design doc at:
https://goo.gl/4npTMU


Diffs
-

  src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a 
  src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 

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


Testing
---

make check

lots of manual testing (using Postman, REST client)


Thanks,

Marco Massenzio

Review Request 36389: [WIP] Enable remote execution of arbitrary command.

[Marco Massenzio]

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

Review request for mesos, Benjamin Hindman and Cody Maloney.


Repository: mesos


Description
---

Jira: MESOS-2830

Under certain (maintenance) circumnstance, it may be necessary
(or desirable) to execute arbitrary operator's commands on the
slave (the entire fleet or a subset thereof) bypassing the Mesos
Task execution mechanism; this may typically be necessary for
maintenance and/or emergency actions.

This patch adds an HTTP endpoint (/execute) which accepts a
JSON-encoded CommandInfo structure and executes the given
command (with optional arguments).

The output of the command (along with potentially any stderr
messages) is returned JSON-encoded in the Response.

For more details, see the design doc at:
https://goo.gl/4npTMU


Diffs
-

  src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a 
  src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 

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


Testing
---

make check

lots of manual testing (using Postman, REST client)


Thanks,

Marco Massenzio