Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 5:01 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Add reviewer.


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


Repository: mesos


Description
---

During parsing of resources in the startup flags of mesos agent, all
the valid resources (including empty resources) are accumulated in a
vector of Resource protobufs.


Diffs
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 5:01 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Add reviewer.


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


Repository: mesos


Description
---

Text based representation of disk resources can indicate source type
(ie. PATH or MOUNT) and its root. This makes it closer to the JSON
representation.


Diffs
-

  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 5 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Add reviewer.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51978, 51991, 51990, 51623]

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 Sept. 18, 2016, 9:11 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 18, 2016, 9:11 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to support task groups.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran this command:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 
> --taskgroup=file:///home/abhishek/taskgroup.txt
> 
> `/home/abhishek/taskgroup.txt:`
> {
>   "tasks": [{
>   "name": "sigmtdroid",
>   "task_id": {
>   "value": "sigmoid"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 1
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }, {
>   "name": "sigmteroid",
>   "task_id": {
>   "value": "sigmoid2"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 2
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }]
> }
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50960: Added appc uri fetcher tests.

[Srinivas Brahmaroutu]

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

(Updated Sept. 19, 2016, 3:35 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to fetch using all three modes that 'rkt fetch' supports,
fetch with meta discovery  ex: 'coreos.com/hello:latest',
fetch from a specific  location ex: 'https://github.com/xxx/hello.aci'
fetch from docker registry ex: 'docker://hello-world'.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp 3c1bd33137612de90d65b7af288bb81ac9876c8b 

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


Testing (updated)
---

make check

Found issue that the tests would fail if "rkt" binary is not available. Please 
suggest if I should use some environment variable to selectively run the tests 
under __IF_DEFINED_RKT


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50959: Added appc fetcher plugin to use rkt tool.

[Srinivas Brahmaroutu]

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

(Updated Sept. 19, 2016, 3:34 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

AppcFetcherPlugin returns a '.aci' file of a container image.

Fetch supports all three modes that 'rkt fetch' supports, fetch with
meta discovery  ex: 'coreos.com/hello:latest', fetch from a specific
location ex: 'https://github.com/xxx/hello.aci' or fetch from docker
registry ex: 'docker://hello-world'.

Added new fetched plugin for Appc fetch algorithm. The new
AppcFetcherPlugin implements two methods. Method 'rktFetch' is used to
fetch a container image into a store specified at a directory location
using 'rkt fetch' command and method 'rktExport' exports the image
fetched into the directory location as a '.aci' file that is in tar
file format, usinf 'rkt image export' command.


Diffs (updated)
-

  src/Makefile.am 44fd8a216eb70d806f74b6f6acff69a2e55b7ede 
  src/uri/fetcher.hpp 949363ecdb15f5b225db7f3ed67eda52c351bfa1 
  src/uri/fetcher.cpp 904fce5a203c57ef1b8fdda09c81efbcf18f5d2c 
  src/uri/fetchers/rkt.hpp PRE-CREATION 
  src/uri/fetchers/rkt.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50959: Added appc fetcher plugin to use rkt tool.

[Srinivas Brahmaroutu]

> On Sept. 16, 2016, 12:38 a.m., Jie Yu wrote:
> > src/uri/fetchers/appc.cpp, lines 149-151
> > 
> >
> > I suggest we create a class called Rkt which abstracts the rkt tool.
> > ```
> > Rkt rkt = Rkt::create();
> > rkt->fetch(...);
> > rkt->export(...);
> > ```
> > 
> > You can do the os::which check in Rkt::create.

I did make sure that .then construct worksby moving it to a process with Fetch 
and Export methods. Apparently "export" cannot be a function name.


- Srinivas


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


On Sept. 19, 2016, 1:15 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50959/
> ---
> 
> (Updated Sept. 19, 2016, 1:15 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4288
> https://issues.apache.org/jira/browse/MESOS-4288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppcFetcherPlugin returns a '.aci' file of a container image.
> 
> Fetch supports all three modes that 'rkt fetch' supports, fetch with
> meta discovery  ex: 'coreos.com/hello:latest', fetch from a specific
> location ex: 'https://github.com/xxx/hello.aci' or fetch from docker
> registry ex: 'docker://hello-world'.
> 
> Added new fetched plugin for Appc fetch algorithm. The new
> AppcFetcherPlugin implements two methods. Method 'rktFetch' is used to
> fetch a container image into a store specified at a directory location
> using 'rkt fetch' command and method 'rktExport' exports the image
> fetched into the directory location as a '.aci' file that is in tar
> file format, usinf 'rkt image export' command.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 44fd8a216eb70d806f74b6f6acff69a2e55b7ede 
>   src/uri/fetcher.hpp 949363ecdb15f5b225db7f3ed67eda52c351bfa1 
>   src/uri/fetcher.cpp 904fce5a203c57ef1b8fdda09c81efbcf18f5d2c 
>   src/uri/fetchers/rkt.hpp PRE-CREATION 
>   src/uri/fetchers/rkt.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50959/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 52004: Supported docker runtime isolator to be nested aware.

[Guangya Liu]

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



Ditto for appc run time isolator here: https://reviews.apache.org/r/52005/


src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 140 - 141)


s/info/command info 

this can be more accurate.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 231 - 235)


What about merging this with #138 - #142 after update? Seems most of the 
comments are related to that.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 235)


Nit: s/neccessary/necessary


- Guangya Liu


On 九月 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52004/
> ---
> 
> (Updated 九月 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6190
> https://issues.apache.org/jira/browse/MESOS-6190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker runtime isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp 
> ac0397f65bb2a675ad1eae0f7cfa95f10145fc63 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> b589cd691ae6aacd2dcd00878e43d58f15abfe11 
> 
> Diff: https://reviews.apache.org/r/52004/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45491: Refactored subprocess options [1/2].

[Jie Yu]

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




3rdparty/libprocess/include/process/subprocess_base.hpp (lines 144 - 147)


Looks like these three are only available on posix. Can we make sure to at 
least add a TODO and ticket to track to not expose these options on windows 
platforms?



3rdparty/libprocess/include/process/subprocess_base.hpp (line 148)


I think WATCHDOG is a child hook as well. Can you remove that and introduce 
a SUPERVISED child hook?


- Jie Yu


On Sept. 18, 2016, 7:43 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45491/
> ---
> 
> (Updated Sept. 18, 2016, 7:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-5070
> https://issues.apache.org/jira/browse/MESOS-5070
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the subprocess interface supported a several options for the
> child process such as setsid. In order to make the interface more
> flexible we refactored such options into a vector of ChildHooks.
> In order not to allow arbitrary code inside a ChildHook it has to be
> constructed via pre-defined factory methods.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> 57f71d15e6fbb032c8b6c5d0c73a93751022e7e7 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ad623a5ea3b3286983e895010af756f14f51b64 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> acd23c99162f164c02a7781abeb95b3c220ead12 
>   3rdparty/libprocess/src/subprocess.cpp 
> 8ca61f77083ac4b77aa1aec22806e1be43dbfa6c 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 0ff3945d2c722ebc1529265427397c5dfba6854e 
> 
> Diff: https://reviews.apache.org/r/45491/diff/
> 
> 
> Testing
> ---
> 
> Tested entire chain see https://reviews.apache.org/r/45492.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 52004: Supported docker runtime isolator to be nested aware.

[Guangya Liu]

> On 九月 18, 2016, 9:03 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/runtime.cpp, lines 77-82
> > 
> >
> > We can remove this. The default behavior is this.

There is already a patch here 
https://reviews.apache.org/r/49234/diff/2#index_header


- Guangya


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


On 九月 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52004/
> ---
> 
> (Updated 九月 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6190
> https://issues.apache.org/jira/browse/MESOS-6190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker runtime isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp 
> ac0397f65bb2a675ad1eae0f7cfa95f10145fc63 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> b589cd691ae6aacd2dcd00878e43d58f15abfe11 
> 
> Diff: https://reviews.apache.org/r/52004/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 52020: Fixed indent in allocator.

[Guangya Liu]

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

Review request for mesos, Alexander Rukletsov and Joseph Wu.


Repository: mesos


Description
---

Fixed indent in allocator.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
2d56bd011f2c87c67a02d0ae467a4a537d36867e 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 52018: Renamed Hook to ParentHook in Mesos.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45491, 45492, 52015, 52016, 52017, 52018]

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 Sept. 18, 2016, 7:50 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52018/
> ---
> 
> (Updated Sept. 18, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed Hook to ParentHook in Mesos.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 1c170e5d11ef31d468b200c2c4cbd27abeeb418a 
>   src/slave/containerizer/docker.cpp a47e2ed88dcadb211c7f8c92eb4bada348d42780 
>   src/slave/containerizer/mesos/launcher.hpp 
> 61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 
>   src/slave/containerizer/mesos/launcher.cpp 
> 73a0e5f50042b1249224a4a42e2442b7ea51dfff 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> dca827cba4d1ead90dba036c9b94ba5e3a3f82fe 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1c49f568b12f5ad688bb43625de3dcbfd618ca79 
>   src/tests/containerizer/launcher.hpp 
> f87b3ce92c6a5d996995833d79c976db6afe2700 
> 
> Diff: https://reviews.apache.org/r/52018/diff/
> 
> 
> Testing
> ---
> 
> sudo make check + internal ci
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 51411: Added provisioner tests to provision using meta discovery.

[Srinivas Brahmaroutu]

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

(Updated Sept. 19, 2016, 1:16 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added two tests that will test two of the image meta discovery paths.
Test ROOT_MetaDiscoveryAppcImageTest pulls a appc image from quay.io and
the test ROOT_MetaDiscoveryDockerImageTest pulls a docker image from the
docker hub. Both tests use "useMetaDiscovery" flag along with other Appc
image labels to invoke meta discovery algorithm from "rkt" tool.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6ef1c926d7aa942e241a24d4d3838a5f2d7c4bd1 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50960: Added appc uri fetcher tests.

[Srinivas Brahmaroutu]

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

(Updated Sept. 19, 2016, 1:16 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to fetch using all three modes that 'rkt fetch' supports,
fetch with meta discovery  ex: 'coreos.com/hello:latest',
fetch from a specific  location ex: 'https://github.com/xxx/hello.aci'
fetch from docker registry ex: 'docker://hello-world'.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp 3c1bd33137612de90d65b7af288bb81ac9876c8b 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51410: Enabled meta discovery using appc labels.

[Srinivas Brahmaroutu]

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

(Updated Sept. 19, 2016, 1:16 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added appc_labels to mesos-execute so that we can indicate meta
discovery using useMetaDiscovery="true" along with other appc labels
that determines the os, arch of the image we are discovering. This logic
needs to be documented so that user can switch between simple and meta
discovery methods. This implementation is necessary as there is no other
way to determine from the URI.


Diffs (updated)
-

  src/cli/execute.cpp c9f56af7f37d5b79b51f350d6c533714c170e889 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp 
907e761856e8996b72e4231de27fa15e884d52e3 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50959: Added appc fetcher plugin to use rkt tool.

[Srinivas Brahmaroutu]

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

(Updated Sept. 19, 2016, 1:15 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

AppcFetcherPlugin returns a '.aci' file of a container image.

Fetch supports all three modes that 'rkt fetch' supports, fetch with
meta discovery  ex: 'coreos.com/hello:latest', fetch from a specific
location ex: 'https://github.com/xxx/hello.aci' or fetch from docker
registry ex: 'docker://hello-world'.

Added new fetched plugin for Appc fetch algorithm. The new
AppcFetcherPlugin implements two methods. Method 'rktFetch' is used to
fetch a container image into a store specified at a directory location
using 'rkt fetch' command and method 'rktExport' exports the image
fetched into the directory location as a '.aci' file that is in tar
file format, usinf 'rkt image export' command.


Diffs (updated)
-

  src/Makefile.am 44fd8a216eb70d806f74b6f6acff69a2e55b7ede 
  src/uri/fetcher.hpp 949363ecdb15f5b225db7f3ed67eda52c351bfa1 
  src/uri/fetcher.cpp 904fce5a203c57ef1b8fdda09c81efbcf18f5d2c 
  src/uri/fetchers/rkt.hpp PRE-CREATION 
  src/uri/fetchers/rkt.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Guangya Liu]

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




src/cli/execute.cpp (lines 100 - 101)


s/TaskGroupInfo/`TaskGroupInfo`



src/cli/execute.cpp (line 389)


kill this



src/cli/execute.cpp (line 396)


s/.get()./->

Ditto for others



src/cli/execute.cpp (line 558)


why kill this? I did not quite catch @vinod's comments above, but I think 
that we still need this check and also check for task from task group?



src/cli/execute.cpp (lines 777 - 781)


keep align with above `flags.name.isSome() ||`


- Guangya Liu


On 九月 18, 2016, 9:11 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated 九月 18, 2016, 9:11 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to support task groups.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran this command:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 
> --taskgroup=file:///home/abhishek/taskgroup.txt
> 
> `/home/abhishek/taskgroup.txt:`
> {
>   "tasks": [{
>   "name": "sigmtdroid",
>   "task_id": {
>   "value": "sigmoid"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 1
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }, {
>   "name": "sigmteroid",
>   "task_id": {
>   "value": "sigmoid2"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 2
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>  

Re: Review Request 52015: Used {} instead of Hook::None() in libprocess [1/2].

[Qian Zhang]

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




3rdparty/libprocess/include/process/subprocess_base.hpp (lines 173 - 176)


I think you need to remove these codes as well.


- Qian Zhang


On Sept. 19, 2016, 3:49 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52015/
> ---
> 
> (Updated Sept. 19, 2016, 3:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used {} instead of Hook::None() in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> acd23c99162f164c02a7781abeb95b3c220ead12 
> 
> Diff: https://reviews.apache.org/r/52015/diff/
> 
> 
> Testing
> ---
> 
> sudo make check + internal ci
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 52016: Replaces Hook::None() by {} in Mesos [2/2].

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Sept. 19, 2016, 3:48 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52016/
> ---
> 
> (Updated Sept. 19, 2016, 3:48 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaces Hook::None() by {} in Mesos.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.cpp 7c40ebebb983b7977526fee123bf07c86fbb5e50 
>   src/linux/perf.cpp 16a30eac9d346b14f2245128003866b9894bb94a 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
> 2b2c6cf2ba35b48e17a169e7a65380889c311266 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> 5b9d5aaeb008e9b102e86736c9312db0b3266d34 
>   src/slave/containerizer/mesos/launcher.hpp 
> 61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 
> 
> Diff: https://reviews.apache.org/r/52016/diff/
> 
> 
> Testing
> ---
> 
> sudo make check + internal ci
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 52018: Renamed Hook to ParentHook in Mesos.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/launcher.cpp (line 133)


The indent here seems not correct, should be 4 spaces.


- Qian Zhang


On Sept. 19, 2016, 3:50 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52018/
> ---
> 
> (Updated Sept. 19, 2016, 3:50 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed Hook to ParentHook in Mesos.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 1c170e5d11ef31d468b200c2c4cbd27abeeb418a 
>   src/slave/containerizer/docker.cpp a47e2ed88dcadb211c7f8c92eb4bada348d42780 
>   src/slave/containerizer/mesos/launcher.hpp 
> 61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 
>   src/slave/containerizer/mesos/launcher.cpp 
> 73a0e5f50042b1249224a4a42e2442b7ea51dfff 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> dca827cba4d1ead90dba036c9b94ba5e3a3f82fe 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1c49f568b12f5ad688bb43625de3dcbfd618ca79 
>   src/tests/containerizer/launcher.hpp 
> f87b3ce92c6a5d996995833d79c976db6afe2700 
> 
> Diff: https://reviews.apache.org/r/52018/diff/
> 
> 
> Testing
> ---
> 
> sudo make check + internal ci
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 52017: Renamed Hook to parent Hook in libprocess [1/2].

[Qian Zhang]

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




3rdparty/libprocess/include/process/subprocess_base.hpp (line 173)


I think you should remove these code in https://reviews.apache.org/r/52015/ 
rather than in this patch which is only for renaming.



3rdparty/libprocess/src/subprocess.cpp (line 54)


Can we rename `parent_callback` to `parent_setup` to be consistent with 
`child_setup`?


- Qian Zhang


On Sept. 19, 2016, 3:49 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52017/
> ---
> 
> (Updated Sept. 19, 2016, 3:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed Hook to parent Hook in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> 57f71d15e6fbb032c8b6c5d0c73a93751022e7e7 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> acd23c99162f164c02a7781abeb95b3c220ead12 
>   3rdparty/libprocess/src/subprocess.cpp 
> 8ca61f77083ac4b77aa1aec22806e1be43dbfa6c 
> 
> Diff: https://reviews.apache.org/r/52017/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain (see 53018).
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 52006: Supported docker/volume isolator to be nested aware.

[Guangya Liu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 170 - 
172)


I think that you may get some idea from 
https://reviews.apache.org/r/51920/diff/3#0 for how to handle nest container 
for recover.


- Guangya Liu


On 九月 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52006/
> ---
> 
> (Updated 九月 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6193
> https://issues.apache.org/jira/browse/MESOS-6193
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker/volume isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 2cc8e764ff18c95c29598df75cdb370ccf120662 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> af9f3736b487b595e8768e56ce60dc4823db28a1 
> 
> Diff: https://reviews.apache.org/r/52006/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51407: Updated mesos containerizer to checkpoint container runtime information.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 151 - 153)


We we make this a member function of MesosContainerizer. In that way, no 
need to pass in the 'flags'

Binding 'flags' sounds expensive for each reap callas flags can be pretty 
big. You can use
```
.then(defer(self(), [this](...) ...)
```
to avoid binding flags.



src/slave/containerizer/mesos/containerizer.cpp (lines 1344 - 1362)


THis can be combined by simply using state::checkpoint(...)



src/slave/containerizer/mesos/containerizer.cpp (line 1353)


I'd have a helper path function for pid file as well.



src/slave/containerizer/mesos/containerizer.cpp (lines 1791 - 1797)


Hum, should we do the deletion here if we don't even sure the processes are 
killed properly?

I think my suggestion on RuntimePath for container is:
1) we create the RuntimePath as the first thing in 'launch', even before we 
call any provisioner/isolator functions.
2) we checkpoint the pid right after fork
3) we delete the RuntimePath after the destroy is successful.

The invariants we have if using the above way are:
1) If RuntimePath exists, we know that provisioner/isolator prepare might 
be called, so cleanup is necessary during recovery.
2) If RuntimePath does not exist, we know that all cleanups have been done 
properly and we no longer need to worry about cleanup.
3) If pid file exists, we know that the process has been forked.
4) If the pid file does not exist, we may or maynot have process being 
forked.

For the upgrade situation, some checkpointed containers or orphans may not 
have RuntimePath. In such case, we should not create RuntimePath (i.e., do not 
checkpoint pid or exit status) in order to maintain the above invariant. So for 
old containers launched by previous version of agent, there will be no 
RuntimePath for it at any time (this is another invariant).

It's likely that a container has RuntimePath, but launcher does not know 
about it. This is the case where launcher->destroy has been called (and being 
successful), but agent crashes before removing RuntimePath.

It's also likely that a container does not have RuntimePath, but launcher 
knows about it. This is the legacy container case.


- Jie Yu


On Sept. 18, 2016, 11:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51407/
> ---
> 
> (Updated Sept. 18, 2016, 11:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6204
> https://issues.apache.org/jira/browse/MESOS-6204
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes checkpointing both the container pid and the status of
> the container upon exit. This also includes an update to tests to
> account for new 'init' process semantics in a container. That is, the
> name of the init process of the container is now "mesos-containerizer"
> not "sh".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
>   src/tests/containerizer/isolator_tests.cpp 
> 93ce75180520d382f63ce7323be844fe43c6594e 
> 
> Diff: https://reviews.apache.org/r/51407/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52006: Supported docker/volume isolator to be nested aware.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 534)


To be consistent with other messages, I think this should be `": It has 
child container "`.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 535)


I think this should be `" which is not cleaned up yet"` or `" not cleaned 
up yet"`


- Qian Zhang


On Sept. 19, 2016, 1:30 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52006/
> ---
> 
> (Updated Sept. 19, 2016, 1:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6193
> https://issues.apache.org/jira/browse/MESOS-6193
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker/volume isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 2cc8e764ff18c95c29598df75cdb370ccf120662 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> af9f3736b487b595e8768e56ce60dc4823db28a1 
> 
> Diff: https://reviews.apache.org/r/52006/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Jie Yu]

> On Sept. 18, 2016, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 250
> > 
> >
> > fork is linux specific. Please guard all pre_exec_commands with ifdef 
> > linux.
> 
> Jie Yu wrote:
> If we want to keep it on windows as well. I'd suggest we create a 
> `os::spawn` which is similar to os::system, but support argv style. This can 
> be implemented on both windows and linux (on windows, take a look at execvp 
> impl, `_spawnvp` is what you want), on linux, you can do the same as 
> os::system, but uses argv version if needed.
> 
> Jie Yu wrote:
> THis is in fact similar to posix_spawn 
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/posix_spawn.html)
> 
> Kevin Klues wrote:
> I know benh was opposed to creating a version of `os:system()` that took 
> an argv, because this goes against what the libc `system` call does (which 
> just runs the passed in string in a shell).
> 
> `posix_spawn()` looks more like what we want.
> 
> Would you prefer me to do the full blown `spawn()` implementation or just 
> wrap this in an `#ifdef linux` for now?

spawn should be pretty straight forward to implement for both linux and 
windows. I'd prefer just do it to avoid doing a big change to wrap 
pre_exec_commands with ifdef linux.


- Jie


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


On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52011/
> ---
> 
> (Updated Sept. 18, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6075
> https://issues.apache.org/jira/browse/MESOS-6075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we used 'process::subprocess()' to run all of our pre-exec
> commands. However, doing so causes us to (unnecesssarily) initialize
> all of libprocess (and subsequently creating a whole bunch of unused
> threads, etc.) just to run a simple script.
> 
> To avoid this, we now use fork/exec exec directly avoid calling
> 'process:subprocess()'. In the past, we used 'os::system()' here
> to avoid initializing libprocess, but this caused security issues with
> allowing arbitrary shell commands to be appended to root-level
> pre-exec commands that take strings as their last argument (e.g. mount
> --bind  , where target is user supplied and is set to
> "target_dir; rm -rf /"). We now handle this case by invoking `execvp`
> directly (which ensures that exactly one command is invoked and that
> all of the strings passed to it are treated as direct arguments to
> that one command).
> 
> In the fututre, we should consider refactoring
> 'libprocess::subprocess()' to pull the base functionality into stout
> to avoid initializing libprocess for simple use cases like this one.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/52011/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Kevin Klues]

> On Sept. 18, 2016, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 250
> > 
> >
> > fork is linux specific. Please guard all pre_exec_commands with ifdef 
> > linux.
> 
> Jie Yu wrote:
> If we want to keep it on windows as well. I'd suggest we create a 
> `os::spawn` which is similar to os::system, but support argv style. This can 
> be implemented on both windows and linux (on windows, take a look at execvp 
> impl, `_spawnvp` is what you want), on linux, you can do the same as 
> os::system, but uses argv version if needed.
> 
> Jie Yu wrote:
> THis is in fact similar to posix_spawn 
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/posix_spawn.html)

I know benh was opposed to creating a version of `os:system()` that took an 
argv, because this goes against what the libc `system` call does (which just 
runs the passed in string in a shell).

`posix_spawn()` looks more like what we want.

Would you prefer me to do the full blown `spawn()` implementation or just wrap 
this in an `#ifdef linux` for now?


- Kevin


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


On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52011/
> ---
> 
> (Updated Sept. 18, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6075
> https://issues.apache.org/jira/browse/MESOS-6075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we used 'process::subprocess()' to run all of our pre-exec
> commands. However, doing so causes us to (unnecesssarily) initialize
> all of libprocess (and subsequently creating a whole bunch of unused
> threads, etc.) just to run a simple script.
> 
> To avoid this, we now use fork/exec exec directly avoid calling
> 'process:subprocess()'. In the past, we used 'os::system()' here
> to avoid initializing libprocess, but this caused security issues with
> allowing arbitrary shell commands to be appended to root-level
> pre-exec commands that take strings as their last argument (e.g. mount
> --bind  , where target is user supplied and is set to
> "target_dir; rm -rf /"). We now handle this case by invoking `execvp`
> directly (which ensures that exactly one command is invoked and that
> all of the strings passed to it are treated as direct arguments to
> that one command).
> 
> In the fututre, we should consider refactoring
> 'libprocess::subprocess()' to pull the base functionality into stout
> to avoid initializing libprocess for simple use cases like this one.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/52011/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52014: Added `supportsNesting` method to `network/cni` isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51857, 51871, 51986, 52014]

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 Sept. 18, 2016, 7:07 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52014/
> ---
> 
> (Updated Sept. 18, 2016, 7:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `supportsNesting` method to `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 949da8f70fb1cd13d6359780b032cb170693ea3e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/52014/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51407: Updated mesos containerizer to checkpoint container runtime information.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 11:14 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to use newer container->status field as well as add some comments.


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


Repository: mesos


Description
---

This includes checkpointing both the container pid and the status of
the container upon exit. This also includes an update to tests to
account for new 'init' process semantics in a container. That is, the
name of the init process of the container is now "mesos-containerizer"
not "sh".


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
  src/tests/containerizer/isolator_tests.cpp 
93ce75180520d382f63ce7323be844fe43c6594e 

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


Testing
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51406: Updated launch helper to optionally spawn an 'init' process on linux.

[Jie Yu]

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



Can you follow up with tests on this. I think for this, we can test using 
subprocess pretty easily?

We need to verify signal forwarding as well as signal relay to parent. We also 
need to validate the checkpointing.

- Jie Yu


On Sept. 18, 2016, 6:57 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51406/
> ---
> 
> (Updated Sept. 18, 2016, 6:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6088
> https://issues.apache.org/jira/browse/MESOS-6088
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the 'mesos-containerizer launch' binary would simply exec
> into the actual command we wanted to launch after doing some set of
> preperatory work. The problem with this approach, however, is that
> this gives us no opportunity to checkpoint the exit status of the
> command so the agent can recover it in cases where it is offline at
> the time the command completes.
> 
> To compensate for this, we now allow 'mesos-containerizer launch' to
> take an optional '--wait_status_path' parameter, which indicates where
> to checkpint the status of the launched command (as returned by
> 'waitpid()'). In order to do this checkpointing, however, we cannot
> simply exec into the command anymore. Instead we now fork-exec the
> command and reap it once it completes. We then checkpoint its status
> and return it as our own. We call the original process the 'init'
> process of the container and the fork-exec'd process its child.  As a
> side effect of doing things this way, we also have to be careful to
> forward all signals sent to the init process down to the child.
> 
> In a subsequent commit we will update the mesos containerizer to use
> this new functionality.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 859990cb85e9e8c06400397256cfc512f0811800 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/51406/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52013: Added helper functions for container runtime paths.

[Jie Yu]

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




src/slave/containerizer/mesos/paths.hpp (line 41)


s/ForContainer//


- Jie Yu


On Sept. 18, 2016, 6:32 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52013/
> ---
> 
> (Updated Sept. 18, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6204
> https://issues.apache.org/jira/browse/MESOS-6204
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper functions for container runtime paths.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/slave/containerizer/mesos/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52013/diff/
> 
> 
> Testing
> ---
> 
> This patch should be commited with `--author="Gilbert Song 
> "`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52013: Added helper functions for container runtime paths.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/paths.hpp (line 59)


s/ForContainer//

ContainerId indicates that it's for a container.



src/slave/containerizer/mesos/paths.hpp (line 60)


instead of passing in flags, I'd suggest we passing in rootDir (similar to 
what we did in slave/paths.hpp).



src/slave/containerizer/mesos/paths.cpp (line 50)


Let's pull "containers" here as a const in cpp file.



src/slave/containerizer/mesos/paths.cpp (line 61)


Ditto on "status"


- Jie Yu


On Sept. 18, 2016, 6:32 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52013/
> ---
> 
> (Updated Sept. 18, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6204
> https://issues.apache.org/jira/browse/MESOS-6204
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper functions for container runtime paths.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/slave/containerizer/mesos/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52013/diff/
> 
> 
> Testing
> ---
> 
> This patch should be commited with `--author="Gilbert Song 
> "`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51406: Updated launch helper to optionally spawn an 'init' process on linux.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 18, 2016, 6:57 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51406/
> ---
> 
> (Updated Sept. 18, 2016, 6:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6088
> https://issues.apache.org/jira/browse/MESOS-6088
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the 'mesos-containerizer launch' binary would simply exec
> into the actual command we wanted to launch after doing some set of
> preperatory work. The problem with this approach, however, is that
> this gives us no opportunity to checkpoint the exit status of the
> command so the agent can recover it in cases where it is offline at
> the time the command completes.
> 
> To compensate for this, we now allow 'mesos-containerizer launch' to
> take an optional '--wait_status_path' parameter, which indicates where
> to checkpint the status of the launched command (as returned by
> 'waitpid()'). In order to do this checkpointing, however, we cannot
> simply exec into the command anymore. Instead we now fork-exec the
> command and reap it once it completes. We then checkpoint its status
> and return it as our own. We call the original process the 'init'
> process of the container and the fork-exec'd process its child.  As a
> side effect of doing things this way, we also have to be careful to
> forward all signals sent to the init process down to the child.
> 
> In a subsequent commit we will update the mesos containerizer to use
> this new functionality.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 859990cb85e9e8c06400397256cfc512f0811800 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/51406/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52012: Reverted "Added extra functions to the 'Launcher' abstraction.".

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 18, 2016, 6:31 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52012/
> ---
> 
> (Updated Sept. 18, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6204
> https://issues.apache.org/jira/browse/MESOS-6204
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit bb047cd72936aa1836b6e959127a572c72fb824b.
> 
> We decided to revert this commit in favor of keeping all container
> checkpointing information in the containerizer. A subsequent commit
> will reintroduce this functionality there.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launcher.hpp 
> 61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 
>   src/slave/containerizer/mesos/launcher.cpp 
> 73a0e5f50042b1249224a4a42e2442b7ea51dfff 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> dca827cba4d1ead90dba036c9b94ba5e3a3f82fe 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1c49f568b12f5ad688bb43625de3dcbfd618ca79 
>   src/tests/containerizer/launcher.hpp 
> f87b3ce92c6a5d996995833d79c976db6afe2700 
> 
> Diff: https://reviews.apache.org/r/52012/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Jie Yu]

> On Sept. 18, 2016, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 250
> > 
> >
> > fork is linux specific. Please guard all pre_exec_commands with ifdef 
> > linux.
> 
> Jie Yu wrote:
> If we want to keep it on windows as well. I'd suggest we create a 
> `os::spawn` which is similar to os::system, but support argv style. This can 
> be implemented on both windows and linux (on windows, take a look at execvp 
> impl, `_spawnvp` is what you want), on linux, you can do the same as 
> os::system, but uses argv version if needed.

THis is in fact similar to posix_spawn 
(http://pubs.opengroup.org/onlinepubs/009695399/functions/posix_spawn.html)


- Jie


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


On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52011/
> ---
> 
> (Updated Sept. 18, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6075
> https://issues.apache.org/jira/browse/MESOS-6075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we used 'process::subprocess()' to run all of our pre-exec
> commands. However, doing so causes us to (unnecesssarily) initialize
> all of libprocess (and subsequently creating a whole bunch of unused
> threads, etc.) just to run a simple script.
> 
> To avoid this, we now use fork/exec exec directly avoid calling
> 'process:subprocess()'. In the past, we used 'os::system()' here
> to avoid initializing libprocess, but this caused security issues with
> allowing arbitrary shell commands to be appended to root-level
> pre-exec commands that take strings as their last argument (e.g. mount
> --bind  , where target is user supplied and is set to
> "target_dir; rm -rf /"). We now handle this case by invoking `execvp`
> directly (which ensures that exactly one command is invoked and that
> all of the strings passed to it are treated as direct arguments to
> that one command).
> 
> In the fututre, we should consider refactoring
> 'libprocess::subprocess()' to pull the base functionality into stout
> to avoid initializing libprocess for simple use cases like this one.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/52011/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Jie Yu]

> On Sept. 18, 2016, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 250
> > 
> >
> > fork is linux specific. Please guard all pre_exec_commands with ifdef 
> > linux.

If we want to keep it on windows as well. I'd suggest we create a `os::spawn` 
which is similar to os::system, but support argv style. This can be implemented 
on both windows and linux (on windows, take a look at execvp impl, `_spawnvp` 
is what you want), on linux, you can do the same as os::system, but uses argv 
version if needed.


- Jie


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


On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52011/
> ---
> 
> (Updated Sept. 18, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6075
> https://issues.apache.org/jira/browse/MESOS-6075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we used 'process::subprocess()' to run all of our pre-exec
> commands. However, doing so causes us to (unnecesssarily) initialize
> all of libprocess (and subsequently creating a whole bunch of unused
> threads, etc.) just to run a simple script.
> 
> To avoid this, we now use fork/exec exec directly avoid calling
> 'process:subprocess()'. In the past, we used 'os::system()' here
> to avoid initializing libprocess, but this caused security issues with
> allowing arbitrary shell commands to be appended to root-level
> pre-exec commands that take strings as their last argument (e.g. mount
> --bind  , where target is user supplied and is set to
> "target_dir; rm -rf /"). We now handle this case by invoking `execvp`
> directly (which ensures that exactly one command is invoked and that
> all of the strings passed to it are treated as direct arguments to
> that one command).
> 
> In the fututre, we should consider refactoring
> 'libprocess::subprocess()' to pull the base functionality into stout
> to avoid initializing libprocess for simple use cases like this one.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/52011/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Guangya Liu]

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




src/cli/execute.cpp (line 98)


How about:

s/taskGroup/task_group



src/cli/execute.cpp (line 99)


s/taskgroup/task_group



src/cli/execute.cpp (line 468)


4 spaces



src/cli/execute.cpp (lines 479 - 484)


I think you can leverage 
https://github.com/apache/mesos/blob/master/src/v1/mesos.cpp#L477 here



src/cli/execute.cpp (line 785)


s/taskgroup/task_group


- Guangya Liu


On 九月 18, 2016, 9:11 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated 九月 18, 2016, 9:11 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to support task groups.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran this command:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 
> --taskgroup=file:///home/abhishek/taskgroup.txt
> 
> `/home/abhishek/taskgroup.txt:`
> {
>   "tasks": [{
>   "name": "sigmtdroid",
>   "task_id": {
>   "value": "sigmoid"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 1
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }, {
>   "name": "sigmteroid",
>   "task_id": {
>   "value": "sigmoid2"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 2
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>  

Re: Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Jie Yu]

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




src/slave/containerizer/mesos/launch.cpp (lines 264 - 265)


argument alignment.



src/slave/containerizer/mesos/launch.cpp (line 273)


either call it 'status' or 'waitStatus'


- Jie Yu


On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52011/
> ---
> 
> (Updated Sept. 18, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6075
> https://issues.apache.org/jira/browse/MESOS-6075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we used 'process::subprocess()' to run all of our pre-exec
> commands. However, doing so causes us to (unnecesssarily) initialize
> all of libprocess (and subsequently creating a whole bunch of unused
> threads, etc.) just to run a simple script.
> 
> To avoid this, we now use fork/exec exec directly avoid calling
> 'process:subprocess()'. In the past, we used 'os::system()' here
> to avoid initializing libprocess, but this caused security issues with
> allowing arbitrary shell commands to be appended to root-level
> pre-exec commands that take strings as their last argument (e.g. mount
> --bind  , where target is user supplied and is set to
> "target_dir; rm -rf /"). We now handle this case by invoking `execvp`
> directly (which ensures that exactly one command is invoked and that
> all of the strings passed to it are treated as direct arguments to
> that one command).
> 
> In the fututre, we should consider refactoring
> 'libprocess::subprocess()' to pull the base functionality into stout
> to avoid initializing libprocess for simple use cases like this one.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/52011/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Jie Yu]

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




src/slave/containerizer/mesos/launch.cpp (line 246)


fork is linux specific. Please guard all pre_exec_commands with ifdef linux.


- Jie Yu


On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52011/
> ---
> 
> (Updated Sept. 18, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6075
> https://issues.apache.org/jira/browse/MESOS-6075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we used 'process::subprocess()' to run all of our pre-exec
> commands. However, doing so causes us to (unnecesssarily) initialize
> all of libprocess (and subsequently creating a whole bunch of unused
> threads, etc.) just to run a simple script.
> 
> To avoid this, we now use fork/exec exec directly avoid calling
> 'process:subprocess()'. In the past, we used 'os::system()' here
> to avoid initializing libprocess, but this caused security issues with
> allowing arbitrary shell commands to be appended to root-level
> pre-exec commands that take strings as their last argument (e.g. mount
> --bind  , where target is user supplied and is set to
> "target_dir; rm -rf /"). We now handle this case by invoking `execvp`
> directly (which ensures that exactly one command is invoked and that
> all of the strings passed to it are treated as direct arguments to
> that one command).
> 
> In the fututre, we should consider refactoring
> 'libprocess::subprocess()' to pull the base functionality into stout
> to avoid initializing libprocess for simple use cases like this one.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
> 
> Diff: https://reviews.apache.org/r/52011/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51720: Updated tests to properly set 'flags.launcher' with correct value.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 18, 2016, 6:10 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51720/
> ---
> 
> (Updated Sept. 18, 2016, 6:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Bugs: MESOS-6141
> https://issues.apache.org/jira/browse/MESOS-6141
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In some of our tests we manually create a 'PosixLauncher' rather than
> relying on the value of 'flags.launcher' to decide which type of
> launcher to create. Since calls to 'CreateSlaveFlags()' set
> 'flags.launcher' to 'linux' by default, there was a discrepency in
> what the flags said, and what actual launcher type we were creating.
> 
> This commit fixes this to explicitly set 'flags.launcher' to the
> appropriate type.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> c8497c9d4253e30a0b1bcd31b27a64255961931e 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/51720/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51405: Updated some tests to use the CreateSlaveFlags() helper.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 18, 2016, 6:51 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51405/
> ---
> 
> (Updated Sept. 18, 2016, 6:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6050
> https://issues.apache.org/jira/browse/MESOS-6050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change was motivated by the desire to set the new `runtime_dir`
> flag properly in these tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/51405/diff/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ src/mesos-tests
> $ sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52010: Supported filesystem linux isolator to be nested aware.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 245)


I think we can still call parseExecutorRunPath first. Then, we can call 
parseContainerPath (in containerizer paths.hpp) to get the containerId



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 255 - 259)


I think we should deal with unknown orphans for nested container, but not 
top level container (because it might belong to DockerContainerizer).

So if the container is a nested container, we still create Info for unkonwn 
orphan and call cleanup immediately after.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 293 - 300)


Hum, we should enforce that in containerizer, rather than doing this check 
everywhere.

Can we make sure, in containerizer, that if a parent container is being 
destroyed (state == DESTROYING), no child container can be launched?



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 331 - 334)


This does not sound right, we could have persistent volumes for nested 
container as well.


- Jie Yu


On Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52010/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported filesystem linux isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> 0a85935550e36c9142d845465cfa70a1634a647a 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ea418252956c8089acc5a491888ed7f6df6cafcd 
> 
> Diff: https://reviews.apache.org/r/52010/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51978: Refactored commandScheduler in cli/execute.cpp to take TaskInfo.

[Guangya Liu]

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




src/cli/execute.cpp (lines 526 - 538)


I like to simplify the logic as:

```
if (volumes.isSome()) {
  foreach (const Volume& volume, volumes.get()) {
containerInfo.add_volumes()->CopyFrom(volume);
  }
}
```



src/cli/execute.cpp (line 544)


```
volumes.isNone || volumes->empty()
```



src/cli/execute.cpp (line 778)


move this to L789


- Guangya Liu


On 九月 18, 2016, 9:06 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51978/
> ---
> 
> (Updated 九月 18, 2016, 9:06 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This refactoring was needed for better accomodating pod support
> for mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51978/diff/
> 
> 
> Testing
> ---
> 
> Manually Ran:
> mesos-execute --master=127.0.0.1:5050 --name="Hello1" --command="echo hello" 
> --docker_image="ubuntu" --containerizer="docker"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 52009: Added slave path helper method 'getNestedSandboxPath()'.

[Jie Yu]

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




src/slave/paths.hpp (line 206)


Why need this? Can we use a combination of getExecutorRunPath and 
getSandboxPath to achieve the same?


- Jie Yu


On Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52009/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added slave path helper method 'getNestedSandboxPath()'.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
>   src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 
> 
> Diff: https://reviews.apache.org/r/52009/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52008: Added slave helper function for nested containers 'getSandboxPath()'.

[Jie Yu]

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



Similarily, can you move those to src/slave/containerizer/mesos/paths.hpp|cpp?


src/slave/paths.hpp (line 202)


What's base? Can you add some comments?


- Jie Yu


On Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52008/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added slave helper function for nested containers 'getSandboxPath()'.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
>   src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 
> 
> Diff: https://reviews.apache.org/r/52008/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52007: Updated 'parseExecutorRunPath()' for nested sandbox support.

[Jie Yu]

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



Discussed offline. Looks like the sandbox related method should not be in 
slave/paths.hpp|cpp. Task/Executor related path functions should be in there, 
but sandbox related should be moved to 
src/slave/containerizer/mesos/paths.hpp|cpp

- Jie Yu


On Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52007/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated 'parseExecutorRunPath()' for nested sandbox support.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ea418252956c8089acc5a491888ed7f6df6cafcd 
>   src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
>   src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 
>   src/tests/paths_tests.cpp 0671ee25b484cacf08c9a20ee6eba88e6f14fa97 
> 
> Diff: https://reviews.apache.org/r/52007/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52007: Updated 'parseExecutorRunPath()' for nested sandbox support.

[Jie Yu]

> On Sept. 18, 2016, 9:36 p.m., Jie Yu wrote:
> > src/slave/paths.cpp, line 106
> > 
> >
> > I'd add a comment here, explaining the layout:
> > ```
> > For a nested container x.y.z, the sandbox layout is the following:
> > .../runs/x/.containers/y/.containers/z
> > ```

In fact, a second thought on this. For a nested container x.y.z, can we use the 
following layout consistently:
```
.../runs/x/.containers/x.y/.containers/x.y.z/
```


- Jie


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


On Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52007/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated 'parseExecutorRunPath()' for nested sandbox support.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ea418252956c8089acc5a491888ed7f6df6cafcd 
>   src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
>   src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 
>   src/tests/paths_tests.cpp 0671ee25b484cacf08c9a20ee6eba88e6f14fa97 
> 
> Diff: https://reviews.apache.org/r/52007/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52007: Updated 'parseExecutorRunPath()' for nested sandbox support.

[Jie Yu]

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



Unit test?


src/slave/paths.hpp (lines 97 - 103)


Should we rename this to SandboxPath?



src/slave/paths.cpp (line 106)


I suggest we make `.containers` a constant. I imagine many places in the 
code will use that. FOr instance, we might want to make sure there is no 
`container_path` equals to `.containers`.

I suggest we rename the exiting `CONTAINERS_DIR` to `EXECUTOR_RUNS_DIR` and 
make this `CONTAINERS_DIR`



src/slave/paths.cpp (lines 106 - 122)


Do you also want to update the comments about the layout in paths.hpp?



src/slave/paths.cpp (line 106)


I'd add a comment here, explaining the layout:
```
For a nested container x.y.z, the sandbox layout is the following:
.../runs/x/.containers/y/.containers/z
```



src/slave/paths.cpp (line 107)


Instead of using this variable, I'd prefer using mod operator:
```
if (tokens.size() > 8 && tokens[8] == CONTAINERS_DIR) {
  for (...) {
if (i % 2 == 0) {
  if (tokens[i] != CONTAINERS_DIR) {
break;
  }
} else {
  ContainerID id;
  id.set_value(tokens[i]);
  is.mutable_parent()->CopyFrom(path.containerId);
  path.containerId = id;
}
  }
}
```


- Jie Yu


On Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52007/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated 'parseExecutorRunPath()' for nested sandbox support.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ea418252956c8089acc5a491888ed7f6df6cafcd 
>   src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
>   src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 
>   src/tests/paths_tests.cpp 0671ee25b484cacf08c9a20ee6eba88e6f14fa97 
> 
> Diff: https://reviews.apache.org/r/52007/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Abhishek Dasgupta]

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

(Updated Sept. 18, 2016, 9:11 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Updated mesos-execute to support task groups.


Diffs (updated)
-

  src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 

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


Testing
---

On Ubuntu 16.04:
sudo make -j4

and manually ran this command:
**Successful**
mesos-execute --master=127.0.0.1:5050 
--taskgroup=file:///home/abhishek/taskgroup.txt

`/home/abhishek/taskgroup.txt:`
{
"tasks": [{
"name": "sigmtdroid",
"task_id": {
"value": "sigmoid"
},
"agent_id": {
"value": ""
},
"resources": [{
"name": "cpus",
"type": "SCALAR",
"scalar": {
"value": 1
},
"role": "*"
}, {
"name": "mem",
"type": "SCALAR",
"scalar": {
"value": 32
},
"role": "*"
}],
"command": {
"value": "sleep 1000"
},
"volumes": [{
"container_path": "/mnt/volume",
"mode": "RW",
"source": {
"docker_volume": {
"driver": "volume_driver",
"docker_options": {
"parameter": [{
"key": "key",
"value": "value"
}]
},
"name": "volume_name"
},
"type": "DOCKER_VOLUME"
}
}]
}, {
"name": "sigmteroid",
"task_id": {
"value": "sigmoid2"
},
"agent_id": {
"value": ""
},
"resources": [{
"name": "cpus",
"type": "SCALAR",
"scalar": {
"value": 2
},
"role": "*"
}, {
"name": "mem",
"type": "SCALAR",
"scalar": {
"value": 32
},
"role": "*"
}],
"command": {
"value": "sleep 1000"
},
"volumes": [{
"container_path": "/mnt/volume",
"mode": "RW",
"source": {
"docker_volume": {
"driver": "volume_driver",
"docker_options": {
"parameter": [{
"key": "key",
"value": "value"
}]
},
"name": "volume_name"
},
"type": "DOCKER_VOLUME"
}
}]
}]
}


Thanks,

Abhishek Dasgupta

Re: Review Request 51990: Added << operator to stringify v1::TaskGroupInfo protobuf.

[Abhishek Dasgupta]

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

(Updated Sept. 18, 2016, 9:08 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description (updated)
---

This patch is needed as Flags add() function needs to stringify
v1::TaskGroupInfo before it is parsed to a variable of that
type.


Diffs (updated)
-

  include/mesos/v1/mesos.hpp 936af77f95ccbdc333452a1946187a81222e480f 
  src/v1/mesos.cpp c6a8799648af488d11a48e0c8841b98313c7dff1 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 51991: Added parse function in flags namespace for v1::TaskGroupInfo protobuf.

[Abhishek Dasgupta]

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

(Updated Sept. 18, 2016, 9:07 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added parse function in flags namespace for v1::TaskGroupInfo protobuf.


Diffs (updated)
-

  src/common/parse.hpp 51582a4f8943296799d71d0a8d7812787fd1b2c2 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 51978: Refactored commandScheduler in cli/execute.cpp to take TaskInfo.

[Abhishek Dasgupta]

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

(Updated Sept. 18, 2016, 9:06 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description (updated)
---

This refactoring was needed for better accomodating pod support
for mesos-execute.


Diffs (updated)
-

  src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 

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


Testing
---

Manually Ran:
mesos-execute --master=127.0.0.1:5050 --name="Hello1" --command="echo hello" 
--docker_image="ubuntu" --containerizer="docker"


Thanks,

Abhishek Dasgupta

Re: Review Request 52010: Supported filesystem linux isolator to be nested aware.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52003, 52004, 52005, 52006, 52007, 52008, 52009, 52010]

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 Sept. 18, 2016, 5:31 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52010/
> ---
> 
> (Updated Sept. 18, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6191
> https://issues.apache.org/jira/browse/MESOS-6191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported filesystem linux isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> 0a85935550e36c9142d845465cfa70a1634a647a 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ea418252956c8089acc5a491888ed7f6df6cafcd 
> 
> Diff: https://reviews.apache.org/r/52010/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52004: Supported docker runtime isolator to be nested aware.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 77 - 82)


We can remove this. The default behavior is this.


- Jie Yu


On Sept. 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52004/
> ---
> 
> (Updated Sept. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6190
> https://issues.apache.org/jira/browse/MESOS-6190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker runtime isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp 
> ac0397f65bb2a675ad1eae0f7cfa95f10145fc63 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> b589cd691ae6aacd2dcd00878e43d58f15abfe11 
> 
> Diff: https://reviews.apache.org/r/52004/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52006: Supported docker/volume isolator to be nested aware.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 166 - 
168)


Is this correct? What about nested containers?


- Jie Yu


On Sept. 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52006/
> ---
> 
> (Updated Sept. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6193
> https://issues.apache.org/jira/browse/MESOS-6193
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker/volume isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 2cc8e764ff18c95c29598df75cdb370ccf120662 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> af9f3736b487b595e8768e56ce60dc4823db28a1 
> 
> Diff: https://reviews.apache.org/r/52006/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52005: Supported appc/runtime isolator to be nested aware.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 111 - 112)


Ditto on fixing all the comments in this file.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 136)


What other cases?



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 202)


Ditto.


- Jie Yu


On Sept. 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52005/
> ---
> 
> (Updated Sept. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6192
> https://issues.apache.org/jira/browse/MESOS-6192
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported appc/runtime isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.hpp 
> c25b0eeff44d66c2045fd8daf0feb8ea3db718e4 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
> a96298b801e8ef217dc0e88187b527e3c43a338e 
> 
> Diff: https://reviews.apache.org/r/52005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52004: Supported docker runtime isolator to be nested aware.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 138 - 142)


This needs to be updated.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 164)


What other cases? Can you be more specific here.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 230 - 232)


The comment here needs to be updated as well. Please do a sweep to fix all 
the comments in this file.


- Jie Yu


On Sept. 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52004/
> ---
> 
> (Updated Sept. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6190
> https://issues.apache.org/jira/browse/MESOS-6190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported docker runtime isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp 
> ac0397f65bb2a675ad1eae0f7cfa95f10145fc63 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> b589cd691ae6aacd2dcd00878e43d58f15abfe11 
> 
> Diff: https://reviews.apache.org/r/52004/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52003: Supported volume/image isolator to be nested aware.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 18, 2016, 5:30 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52003/
> ---
> 
> (Updated Sept. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6199
> https://issues.apache.org/jira/browse/MESOS-6199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported volume/image isolator to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/volume/image.hpp 
> 6333e9c881b10184fac2f15f5f4a6f7d781a655f 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> c25205bb80d8008e7879c7a9e6fc274ea0cee5f3 
> 
> Diff: https://reviews.apache.org/r/52003/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 52018: Renamed Hook to ParentHook in Mesos.

[Joerg Schad]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Renamed Hook to ParentHook in Mesos.


Diffs
-

  src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 
  src/slave/container_loggers/lib_logrotate.cpp 
1c170e5d11ef31d468b200c2c4cbd27abeeb418a 
  src/slave/containerizer/docker.cpp a47e2ed88dcadb211c7f8c92eb4bada348d42780 
  src/slave/containerizer/mesos/launcher.hpp 
61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 
  src/slave/containerizer/mesos/launcher.cpp 
73a0e5f50042b1249224a4a42e2442b7ea51dfff 
  src/slave/containerizer/mesos/linux_launcher.hpp 
dca827cba4d1ead90dba036c9b94ba5e3a3f82fe 
  src/slave/containerizer/mesos/linux_launcher.cpp 
1c49f568b12f5ad688bb43625de3dcbfd618ca79 
  src/tests/containerizer/launcher.hpp f87b3ce92c6a5d996995833d79c976db6afe2700 

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


Testing
---

sudo make check + internal ci


Thanks,

Joerg Schad

Review Request 52017: Renamed Hook to parent Hook in libprocess [1/2].

[Joerg Schad]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Renamed Hook to parent Hook in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
57f71d15e6fbb032c8b6c5d0c73a93751022e7e7 
  3rdparty/libprocess/include/process/subprocess_base.hpp 
acd23c99162f164c02a7781abeb95b3c220ead12 
  3rdparty/libprocess/src/subprocess.cpp 
8ca61f77083ac4b77aa1aec22806e1be43dbfa6c 

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


Testing
---

tested entire chain (see 53018).


Thanks,

Joerg Schad

Re: Review Request 52015: Used {} instead of Hook::None() in libprocess [1/2].

[Joerg Schad]

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

(Updated Sept. 18, 2016, 7:49 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Used {} instead of Hook::None() in libprocess [1/2].


Repository: mesos


Description
---

Used {} instead of Hook::None() in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
acd23c99162f164c02a7781abeb95b3c220ead12 

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


Testing
---

sudo make check + internal ci


Thanks,

Joerg Schad

Review Request 52016: Replaces Hook::None() by {} in Mesos [2/2].

[Joerg Schad]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Replaces Hook::None() by {} in Mesos.


Diffs
-

  src/launcher/posix/executor.cpp 7c40ebebb983b7977526fee123bf07c86fbb5e50 
  src/linux/perf.cpp 16a30eac9d346b14f2245128003866b9894bb94a 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
2b2c6cf2ba35b48e17a169e7a65380889c311266 
  src/slave/containerizer/mesos/isolators/posix/disk.cpp 
5b9d5aaeb008e9b102e86736c9312db0b3266d34 
  src/slave/containerizer/mesos/launcher.hpp 
61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 

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


Testing
---

sudo make check + internal ci


Thanks,

Joerg Schad

Review Request 52015: Used {} instead of Hook::None() in libprocess.

[Joerg Schad]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Used {} instead of Hook::None() in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
acd23c99162f164c02a7781abeb95b3c220ead12 

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


Testing
---

sudo make check + internal ci


Thanks,

Joerg Schad

Re: Review Request 45492: Used ChildHooks in Mesos [2/2].

[Joerg Schad]

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

(Updated Sept. 18, 2016, 7:46 p.m.)


Review request for mesos, Alexander Rukletsov and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

We now use the new ChildHooks instead of explicit options such
as setsid.


Diffs (updated)
-

  src/docker/docker.cpp 49a1b2f7a4d7ef3e40bbfe43834ad9200a35740d 
  src/health-check/health_checker.cpp 8beb99cb5f2630c3a5a3821b665952f714f53545 
  src/launcher/posix/executor.cpp 7c40ebebb983b7977526fee123bf07c86fbb5e50 
  src/linux/perf.cpp 16a30eac9d346b14f2245128003866b9894bb94a 
  src/slave/container_loggers/lib_logrotate.cpp 
1c170e5d11ef31d468b200c2c4cbd27abeeb418a 
  src/slave/containerizer/docker.cpp a47e2ed88dcadb211c7f8c92eb4bada348d42780 
  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
2b2c6cf2ba35b48e17a169e7a65380889c311266 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
359479083894e887647a694a1a133dce44817073 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
55a06cfe1edbb7699c04101f78cc4979176e33f6 
  src/slave/containerizer/mesos/isolators/posix/disk.cpp 
5b9d5aaeb008e9b102e86736c9312db0b3266d34 
  src/slave/containerizer/mesos/launcher.cpp 
73a0e5f50042b1249224a4a42e2442b7ea51dfff 
  src/slave/containerizer/mesos/linux_launcher.cpp 
1c49f568b12f5ad688bb43625de3dcbfd618ca79 
  src/tests/containerizer/capabilities_tests.cpp 
3ba46404c389385e02d2a1789e33dab3e8f15902 
  src/tests/containerizer/launch_tests.cpp 
76d71b3088a7bec3b3f8546c197f65557f464028 
  src/tests/containerizer/ns_tests.cpp e52ebbe93cd80236ca865a137de4251f25d55b05 
  src/tests/containerizer/port_mapping_tests.cpp 
7fac6ca1550acf54616fb4cef2eab1335f5e9760 

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


Testing
---

sudo Make check + internal ci


Thanks,

Joerg Schad

Re: Review Request 45491: Refactored subprocess options [1/2].

[Joerg Schad]

> On Sept. 17, 2016, 4:11 a.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess_base.hpp, lines 402-403
> > 
> >
> > Hum, i don't like ChildHook::None(). Have you tried if ```const 
> > vector& child_hooks = {}``` works or not?
> 
> Joerg Schad wrote:
> This is currently consistent with Hook::None(). Do you have a strong 
> opinion?
> 
> Jie Yu wrote:
> Yeah, I'd like to cleanup Hook::None() as well. This caused some 
> confusion to me before. You can clean up the Hook::None() in a separate patch.

https://reviews.apache.org/r/52015/
https://reviews.apache.org/r/52016/


> On Sept. 17, 2016, 4:11 a.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess_base.hpp, line 314
> > 
> >
> > I suggest we rename Hook to ParentHook. You can do that in a separate 
> > patch.
> 
> Joerg Schad wrote:
> Sound good. Will be done in a seperate patch.

https://reviews.apache.org/r/52017/
https://reviews.apache.org/r/52018/


- Joerg


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


On Sept. 18, 2016, 7:43 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45491/
> ---
> 
> (Updated Sept. 18, 2016, 7:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-5070
> https://issues.apache.org/jira/browse/MESOS-5070
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the subprocess interface supported a several options for the
> child process such as setsid. In order to make the interface more
> flexible we refactored such options into a vector of ChildHooks.
> In order not to allow arbitrary code inside a ChildHook it has to be
> constructed via pre-defined factory methods.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> 57f71d15e6fbb032c8b6c5d0c73a93751022e7e7 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ad623a5ea3b3286983e895010af756f14f51b64 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> acd23c99162f164c02a7781abeb95b3c220ead12 
>   3rdparty/libprocess/src/subprocess.cpp 
> 8ca61f77083ac4b77aa1aec22806e1be43dbfa6c 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 0ff3945d2c722ebc1529265427397c5dfba6854e 
> 
> Diff: https://reviews.apache.org/r/45491/diff/
> 
> 
> Testing
> ---
> 
> Tested entire chain see https://reviews.apache.org/r/45492.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45491: Refactored subprocess options [1/2].

[Joerg Schad]

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

(Updated Sept. 18, 2016, 7:43 p.m.)


Review request for mesos, Alexander Rukletsov and Jie Yu.


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


Repository: mesos


Description
---

Previously the subprocess interface supported a several options for the
child process such as setsid. In order to make the interface more
flexible we refactored such options into a vector of ChildHooks.
In order not to allow arbitrary code inside a ChildHook it has to be
constructed via pre-defined factory methods.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
57f71d15e6fbb032c8b6c5d0c73a93751022e7e7 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
2ad623a5ea3b3286983e895010af756f14f51b64 
  3rdparty/libprocess/include/process/subprocess_base.hpp 
acd23c99162f164c02a7781abeb95b3c220ead12 
  3rdparty/libprocess/src/subprocess.cpp 
8ca61f77083ac4b77aa1aec22806e1be43dbfa6c 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
0ff3945d2c722ebc1529265427397c5dfba6854e 

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


Testing
---

Tested entire chain see https://reviews.apache.org/r/45492.


Thanks,

Joerg Schad

Review Request 52014: Added `supportsNesting` method to `network/cni` isolator.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Joseph Wu.


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


Repository: mesos


Description
---

Added `supportsNesting` method to `network/cni` isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
949da8f70fb1cd13d6359780b032cb170693ea3e 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
359479083894e887647a694a1a133dce44817073 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51406: Updated launch helper to optionally spawn an 'init' process on linux.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:57 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated testing done.


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


Repository: mesos


Description
---

Previously the 'mesos-containerizer launch' binary would simply exec
into the actual command we wanted to launch after doing some set of
preperatory work. The problem with this approach, however, is that
this gives us no opportunity to checkpoint the exit status of the
command so the agent can recover it in cases where it is offline at
the time the command completes.

To compensate for this, we now allow 'mesos-containerizer launch' to
take an optional '--wait_status_path' parameter, which indicates where
to checkpint the status of the launched command (as returned by
'waitpid()'). In order to do this checkpointing, however, we cannot
simply exec into the command anymore. Instead we now fork-exec the
command and reap it once it completes. We then checkpoint its status
and return it as our own. We call the original process the 'init'
process of the container and the fork-exec'd process its child.  As a
side effect of doing things this way, we also have to be careful to
forward all signals sent to the init process down to the child.

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs
-

  src/slave/containerizer/mesos/launch.hpp 
859990cb85e9e8c06400397256cfc512f0811800 
  src/slave/containerizer/mesos/launch.cpp 
fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 

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


Testing (updated)
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51443: Updated 'launcher' to properly reap checkpointed exit status on linux.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:55 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

In order to properly reap the new container 'init' process, we need
introduce a new 'Launcher::wait' command that knows how to reap the
'init' process directly and then retreive its exit status from the
checkpoint (if necessary).

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs
-

  src/slave/containerizer/mesos/launcher.hpp 
0eae09515d550a2c71ae1414d4a22943f1d09db9 
  src/slave/containerizer/mesos/launcher.cpp 
413a8afdc56127a58c9599c436d17d6c98e62434 
  src/slave/containerizer/mesos/linux_launcher.hpp 
8fbe1e9742df85b0fc6de46ac81a0c064c845a63 
  src/slave/containerizer/mesos/linux_launcher.cpp 
7377316776646e3d660086da3c0d5b494ce8ace4 
  src/tests/containerizer/launcher.hpp 94c62b761a17436841bd19f3bf622cc8f1047876 
  src/tests/containerizer/launcher.cpp a92d9890f0931425d69ef8ce0896d081b8722079 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51405: Updated some tests to use the CreateSlaveFlags() helper.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:51 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Removed dependence on 51442


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


Repository: mesos


Description
---

This change was motivated by the desire to set the new `runtime_dir`
flag properly in these tests.


Diffs
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51991: Added parse function in flags namespace for v1::TaskGroupInfo protobuf.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 17, 2016, 8:55 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51991/
> ---
> 
> (Updated Sept. 17, 2016, 8:55 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added parse function in flags namespace for v1::TaskGroupInfo protobuf.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 51582a4f8943296799d71d0a8d7812787fd1b2c2 
> 
> Diff: https://reviews.apache.org/r/51991/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: [51880, 51879]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On Sept. 18, 2016, 4:55 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51880/
> ---
> 
> (Updated Sept. 18, 2016, 4:55 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to determine disk size for MOUNT disks.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51880/diff/
> 
> 
> Testing
> ---
> 
> All tests including the additional tests in this RR passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51407: Updated mesos containerizer to checkpoint container runtime information.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:40 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated JIRA link


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


Repository: mesos


Description
---

This includes checkpointing both the container pid and the status of
the container upon exit. This also includes an update to tests to
account for new 'init' process semantics in a container. That is, the
name of the init process of the container is now "mesos-containerizer"
not "sh".


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
  src/tests/containerizer/isolator_tests.cpp 
93ce75180520d382f63ce7323be844fe43c6594e 

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


Testing
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51407: Updated mesos containerizer to checkpoint container runtime information.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:36 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on new decision to checkpoint all runtime information about 
containers in the containerizer instead of the launcher.


Summary (updated)
-

Updated mesos containerizer to checkpoint container runtime information.


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


Repository: mesos


Description (updated)
---

This includes checkpointing both the container pid and the status of
the container upon exit. This also includes an update to tests to
account for new 'init' process semantics in a container. That is, the
name of the init process of the container is now "mesos-containerizer"
not "sh".


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
  src/tests/containerizer/isolator_tests.cpp 
93ce75180520d382f63ce7323be844fe43c6594e 

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


Testing (updated)
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

[haosdent huang]

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




src/Makefile.am (line 2270)


Nit: Need use tab instead of spaces between 
`tests/persistent_shared_volume_framework_test.sh` and ``



src/examples/persistent_shared_volume_framework.cpp (lines 207 - 209)


Nit:
```
  "COUNTER=0;"
  "while [ $COUNTER -lt 5 ];"
  "do"
  "  echo " + task_id + ":Writing from main task=$COUNTER"
  "  >> volume/persistent.dat;"
  "  COUNTER=$[COUNTER+1];"
  "  sleep 2;"
  "done");
```



src/examples/persistent_shared_volume_framework.cpp (lines 249 - 251)


Nits:

```
  "COUNTER=0;"
  "while [ $COUNTER -lt 5 ];"
  "do"
  "  tail -n 1 volume/persistent.dat;"
  "  COUNTER=$[COUNTER+1];"
  "  sleep 2;"
  "done");
```



src/tests/examples_tests.cpp (lines 29 - 30)


Nit: Need to add a extra blank line above.


- haosdent huang


On Sept. 15, 2016, 4:47 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Sept. 15, 2016, 4:47 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a persistent volume test framework for shared volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
>   src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 52013: Added helper functions for container runtime paths.

[Kevin Klues]

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

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


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


Repository: mesos


Description
---

Added helper functions for container runtime paths.


Diffs
-

  src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
  src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
  src/slave/containerizer/mesos/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/paths.cpp PRE-CREATION 

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


Testing
---

This patch should be commited with `--author="Gilbert Song 
"`


Thanks,

Kevin Klues

Review Request 52012: Reverted "Added extra functions to the 'Launcher' abstraction.".

[Kevin Klues]

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

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


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


Repository: mesos


Description
---

This reverts commit bb047cd72936aa1836b6e959127a572c72fb824b.

We decided to revert this commit in favor of keeping all container
checkpointing information in the containerizer. A subsequent commit
will reintroduce this functionality there.


Diffs
-

  src/slave/containerizer/mesos/launcher.hpp 
61c2e84a3cebc308a8a65e536fa07fa1cf8f5838 
  src/slave/containerizer/mesos/launcher.cpp 
73a0e5f50042b1249224a4a42e2442b7ea51dfff 
  src/slave/containerizer/mesos/linux_launcher.hpp 
dca827cba4d1ead90dba036c9b94ba5e3a3f82fe 
  src/slave/containerizer/mesos/linux_launcher.cpp 
1c49f568b12f5ad688bb43625de3dcbfd618ca79 
  src/tests/containerizer/launcher.hpp f87b3ce92c6a5d996995833d79c976db6afe2700 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Vinod Kone]

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




src/cli/execute.cpp (line 457)


s/mesos-execute-executor/default-executor/



src/cli/execute.cpp (lines 548 - 559)


just kill this block. not worth the complexity just to do a CHECK



src/cli/execute.cpp (line 797)


this should not be set if "--task_group" is set.



src/cli/execute.cpp (lines 857 - 870)


these should not be set if "--task_group" is set.



src/cli/execute.cpp (lines 899 - 912)


this should not be set if "--task_group" is set.



src/cli/execute.cpp (line 934)


new line


- Vinod Kone


On Sept. 18, 2016, 3:34 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 18, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to support task groups.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran this command:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 
> --taskgroup=file:///home/abhishek/taskgroup.txt
> 
> `/home/abhishek/taskgroup.txt:`
> {
>   "tasks": [{
>   "name": "sigmtdroid",
>   "task_id": {
>   "value": "sigmoid"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 1
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }, {
>   "name": "sigmteroid",
>   "task_id": {
>   "value": "sigmoid2"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 2
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
> 

Re: Review Request 51879: Determine disk size when not specified in static resources.

[haosdent huang]

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




src/slave/containerizer/containerizer.hpp (lines 57 - 60)


I think we should avoid adding the `diskSize` in the definition header of 
`containerizer` because it looks irrelevant. Suppose we would like to modular 
the containerizer, it would make us in trouble as the TODO mentioned below.

```
  // TODO(idownes): Consider making this non-static and moving to
  // containerizer implementations to enable a containerizer to best
  // determine the resources, particularly if containerizeration is
  // delegated.
  static Try resources(const Flags& flags);
```

`fs.hpp`, `resources.cpp` or  `resources_utils.cpp` would be a better place?



src/slave/containerizer/containerizer.cpp (line 169)


Use 0 to represent that determine the disk size automatically a bit 
nonintuitive.
Please update docs/attributes-resources.md if we have to use this way 
eventually.



src/slave/containerizer/containerizer.cpp (lines 171 - 172)


Should be the filesystem `work_dir` mounted instead of `root disk`?



src/slave/containerizer/containerizer.cpp (lines 183 - 244)


I suggest that we move the logic which parse `disk(0)` to resources.cpp or 
resources_utils.cpp.



src/slave/containerizer/containerizer.cpp (line 186)


`!resource.has_disk()`



src/slave/containerizer/containerizer.cpp (lines 213 - 214)


`.disk().source().type()` should be enough?


- haosdent huang


On Sept. 18, 2016, 4:55 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Sept. 18, 2016, 4:55 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate disks with a positive size, we use that
> for the disk resources on the agent. However, --resources can include
> disks with size of 0, which indicates that mesos agent determine the
> size of those disks from the host and uses that information instead.
> Note that this is not allowed for PATH disks since PATH disks can be
> carved into smaller chunks and we cannot assume that the whole
> physical disk is available to the PATH disk.
> 
> With this change, JSON or textual representation for disk resources
> that specify scalar value of 0 would not result in an error, but those
> resources will not be accounted for until a valid size is determined
> for such resources. A scalar value of -1 in JSON or textual formats
> still results in an invalid resource.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51406: Updated launch helper to optionally spawn an 'init' process on linux.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:18 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to remove dependence on libprocess. Also updated to ensure bi-direction 
signal forwarding (i.e. ensure that if the child dies via a signal, then the 
parent kills itself via the same signal).


Summary (updated)
-

Updated launch helper to optionally spawn an 'init' process on linux.


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


Repository: mesos


Description (updated)
---

Previously the 'mesos-containerizer launch' binary would simply exec
into the actual command we wanted to launch after doing some set of
preperatory work. The problem with this approach, however, is that
this gives us no opportunity to checkpoint the exit status of the
command so the agent can recover it in cases where it is offline at
the time the command completes.

To compensate for this, we now allow 'mesos-containerizer launch' to
take an optional '--wait_status_path' parameter, which indicates where
to checkpint the status of the launched command (as returned by
'waitpid()'). In order to do this checkpointing, however, we cannot
simply exec into the command anymore. Instead we now fork-exec the
command and reap it once it completes. We then checkpoint its status
and return it as our own. We call the original process the 'init'
process of the container and the fork-exec'd process its child.  As a
side effect of doing things this way, we also have to be careful to
forward all signals sent to the init process down to the child.

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.hpp 
859990cb85e9e8c06400397256cfc512f0811800 
  src/slave/containerizer/mesos/launch.cpp 
fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52011: Updated launch helper to avoid initializing libprocess.

[Kevin Klues]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Previously, we used 'process::subprocess()' to run all of our pre-exec
commands. However, doing so causes us to (unnecesssarily) initialize
all of libprocess (and subsequently creating a whole bunch of unused
threads, etc.) just to run a simple script.

To avoid this, we now use fork/exec exec directly avoid calling
'process:subprocess()'. In the past, we used 'os::system()' here
to avoid initializing libprocess, but this caused security issues with
allowing arbitrary shell commands to be appended to root-level
pre-exec commands that take strings as their last argument (e.g. mount
--bind  , where target is user supplied and is set to
"target_dir; rm -rf /"). We now handle this case by invoking `execvp`
directly (which ensures that exactly one command is invoked and that
all of the strings passed to it are treated as direct arguments to
that one command).

In the fututre, we should consider refactoring
'libprocess::subprocess()' to pull the base functionality into stout
to avoid initializing libprocess for simple use cases like this one.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 

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


Testing
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51720: Updated tests to properly set 'flags.launcher' with correct value.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:10 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

In some of our tests we manually create a 'PosixLauncher' rather than
relying on the value of 'flags.launcher' to decide which type of
launcher to create. Since calls to 'CreateSlaveFlags()' set
'flags.launcher' to 'linux' by default, there was a discrepency in
what the flags said, and what actual launcher type we were creating.

This commit fixes this to explicitly set 'flags.launcher' to the
appropriate type.


Diffs (updated)
-

  src/tests/container_logger_tests.cpp c8497c9d4253e30a0b1bcd31b27a64255961931e 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing (updated)
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51405: Updated some tests to use the CreateSlaveFlags() helper.

[Kevin Klues]

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

(Updated Sept. 18, 2016, 6:07 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

This change was motivated by the desire to set the new `runtime_dir`
flag properly in these tests.


Diffs (updated)
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing (updated)
---

$ GTEST_FILTER="" make -j check
$ src/mesos-tests
$ sudo src/mesos-tests


Thanks,

Kevin Klues

Review Request 52009: Added slave path helper method 'getNestedSandboxPath()'.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Added slave path helper method 'getNestedSandboxPath()'.


Diffs
-

  src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
  src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52010: Supported filesystem linux isolator to be nested aware.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Supported filesystem linux isolator to be nested aware.


Diffs
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0a85935550e36c9142d845465cfa70a1634a647a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ea418252956c8089acc5a491888ed7f6df6cafcd 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52008: Added slave helper function for nested containers 'getSandboxPath()'.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Added slave helper function for nested containers 'getSandboxPath()'.


Diffs
-

  src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
  src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52006: Supported docker/volume isolator to be nested aware.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Supported docker/volume isolator to be nested aware.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
2cc8e764ff18c95c29598df75cdb370ccf120662 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
af9f3736b487b595e8768e56ce60dc4823db28a1 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52004: Supported docker runtime isolator to be nested aware.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Supported docker runtime isolator to be nested aware.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/runtime.hpp 
ac0397f65bb2a675ad1eae0f7cfa95f10145fc63 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
b589cd691ae6aacd2dcd00878e43d58f15abfe11 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52005: Supported appc/runtime isolator to be nested aware.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Supported appc/runtime isolator to be nested aware.


Diffs
-

  src/slave/containerizer/mesos/isolators/appc/runtime.hpp 
c25b0eeff44d66c2045fd8daf0feb8ea3db718e4 
  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
a96298b801e8ef217dc0e88187b527e3c43a338e 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52003: Supported volume/image isolator to be nested aware.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Supported volume/image isolator to be nested aware.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/image.hpp 
6333e9c881b10184fac2f15f5f4a6f7d781a655f 
  src/slave/containerizer/mesos/isolators/volume/image.cpp 
c25205bb80d8008e7879c7a9e6fc274ea0cee5f3 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51978, 51990, 51991, 51623]

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 Sept. 18, 2016, 3:34 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 18, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to support task groups.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran this command:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 
> --taskgroup=file:///home/abhishek/taskgroup.txt
> 
> `/home/abhishek/taskgroup.txt:`
> {
>   "tasks": [{
>   "name": "sigmtdroid",
>   "task_id": {
>   "value": "sigmoid"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 1
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }, {
>   "name": "sigmteroid",
>   "task_id": {
>   "value": "sigmoid2"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 2
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }]
> }
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Anindya Sinha]

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

(Updated Sept. 18, 2016, 4:55 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added tests to include support for MOUNT and PATH disks when using textual 
representation.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

[Anindya Sinha]

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

(Updated Sept. 18, 2016, 4:55 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Refactored the original change into multiple RRs (see comments) and rebased.


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


Repository: mesos


Description (updated)
---

When static resources indicate disks with a positive size, we use that
for the disk resources on the agent. However, --resources can include
disks with size of 0, which indicates that mesos agent determine the
size of those disks from the host and uses that information instead.
Note that this is not allowed for PATH disks since PATH disks can be
carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  src/slave/containerizer/containerizer.hpp 
f13669d0dfc4ce3287cfe630cabd0470dc765b51 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

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

Review request for mesos.


Repository: mesos


Description
---

During parsing of resources in the startup flags of mesos agent, all
the valid resources (including empty resources) are accumulated in a
vector of Resource protobufs.


Diffs
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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

Review request for mesos.


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

[Anindya Sinha]

> On Sept. 15, 2016, 5:49 p.m., Jiang Yan Xu wrote:
> > The addition of `parseText` and `parseValidResources` feels unnecessary to 
> > me. I think we can reuse `convertJSON()` to achieve the same.
> > 
> > Overall I think the following steps would be simpler and more elegant.
> > 
> > ## Patch 1: Change convertJSON() to return a Try
> > 
> > This way the empty disks in the result are not dropped.
> > 
> > ```
> > vector convertJSON(
> > const JSON::Array& resourcesJSON,
> > const string& defaultRole)
> > ```
> > 
> > I also don't see why `convertJSON()` needs to fail on empty resources since 
> > they are allowed in the regular string formatted resources specification. 
> > Maybe we can have a **patch 0** that changes that?
> > 
> > The current lone call site can just construct `Resources` from 
> > `vector`.
> > 
> > Then make convertJSON a public static member method.
> > 
> > ## Patch 2: Let Resources class answer if a resource is a MOUNT disk, PATH 
> > disk or ROOT disk
> > 
> > e.g.,
> > 
> > ```
> > static bool Resources::isMountDisk(const Resource& resource);
> > ```
> > 
> > These are pretty general purpose and purely for convenience. Add simple 
> > tests to verify these methods.
> > 
> > ## Patch 3: Autodetection for MOUNT and ROOT disks
> > 
> > In `Containerizer::resources()`, process each disk from the result of 
> > `convertJSON()`.
> > 
> > For MOUNT disks that are empty (`isEmpty()`), detect their sizes and modify 
> > the Resource objects.
> > What about the ROOT disk? If `disk:0` is in a plain string we don't do 
> > anything. With the JSON input the operator can choose to not specify a root 
> > disk to have it autodetected, so I guess we don't have to separately deal 
> > with ROOT disks with an explict "zero" size. If all disks have been 
> > processed and no ROOT disk is encountered, we auto-detect it.
> > 
> > How does it sound?

Refactored this RR into 4 separate RRs:
(1) https://reviews.apache.org/r/51999/ (Patch 1): Refactor parsing of 
resources to include all valid resources.
- Modified `convertJSON()` to not fail on empty resources (now is in sync with 
text representation).
- Converted `convertJSON()` to return `Try`.
- `Resources::parse()` is refactored to use a new function 
`Resources::parseText()` that returns `Try` so that existing 
calls to `Resources::parse()` are not affected.
(2) https://reviews.apache.org/r/52001/: Allow text based disk resources to 
indicate source type and root.
(3) https://reviews.apache.org/r/52002/ (Patch 2): Added helper methods to 
determine types of disk resources.
(4) https://reviews.apache.org/r/51879/ (Patch 3): Determine disk size when not 
specified in static resources.
- As discussed offline, we do not add root disk if missing when `--resources` 
does not include the root disk is to allow the possibility of removing the root 
disk should the operator desires.


> On Sept. 15, 2016, 5:49 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, line 177
> > 
> >
> > Call them `disks` to avoid confusion with the volumes as a Mesos 
> > concept.

Updated based on the refactor.


> On Sept. 15, 2016, 5:49 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 260-265
> > 
> >
> > This is only for the ROOT disk, i.e., we only leave some headroom for 
> > the system root partition.
> > 
> > Therefore we probably don't need this method.

Agreed. However, we have 2 cases for rot disks:
a. When no disks are present in `--resources`
b. When `--resources` indicates a disk without source.

I refactored `Containerizer::diskSize()` to take in if it is a root disk and 
add the headroom for root disks only.


- Anindya


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


On Sept. 14, 2016, 2:29 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Sept. 14, 2016, 2:29 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate disks with a positive size, we use that
> for the disk resources on the agent. However, --resources can include
> disks with size of 0, which indicates that mesos agent determine the
> size of those disks from the host and uses that information instead.
> Note that this is not allowed for PATH disks since PATH disks can be
> carved into 

Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

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

Review request for mesos.


Repository: mesos


Description
---

Text based representation of disk resources can indicate source type
(ie. PATH or MOUNT) and its root. This makes it closer to the JSON
representation.


Diffs
-

  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Abhishek Dasgupta]

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

(Updated Sept. 18, 2016, 3:34 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebased and addressed some of Guangya's comments.


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


Repository: mesos


Description
---

Updated mesos-execute to support task groups.


Diffs (updated)
-

  src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 

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


Testing (updated)
---

On Ubuntu 16.04:
sudo make -j4

and manually ran this command:
**Successful**
mesos-execute --master=127.0.0.1:5050 
--taskgroup=file:///home/abhishek/taskgroup.txt

`/home/abhishek/taskgroup.txt:`
{
"tasks": [{
"name": "sigmtdroid",
"task_id": {
"value": "sigmoid"
},
"agent_id": {
"value": ""
},
"resources": [{
"name": "cpus",
"type": "SCALAR",
"scalar": {
"value": 1
},
"role": "*"
}, {
"name": "mem",
"type": "SCALAR",
"scalar": {
"value": 32
},
"role": "*"
}],
"command": {
"value": "sleep 1000"
},
"volumes": [{
"container_path": "/mnt/volume",
"mode": "RW",
"source": {
"docker_volume": {
"driver": "volume_driver",
"docker_options": {
"parameter": [{
"key": "key",
"value": "value"
}]
},
"name": "volume_name"
},
"type": "DOCKER_VOLUME"
}
}]
}, {
"name": "sigmteroid",
"task_id": {
"value": "sigmoid2"
},
"agent_id": {
"value": ""
},
"resources": [{
"name": "cpus",
"type": "SCALAR",
"scalar": {
"value": 2
},
"role": "*"
}, {
"name": "mem",
"type": "SCALAR",
"scalar": {
"value": 32
},
"role": "*"
}],
"command": {
"value": "sleep 1000"
},
"volumes": [{
"container_path": "/mnt/volume",
"mode": "RW",
"source": {
"docker_volume": {
"driver": "volume_driver",
"docker_options": {
"parameter": [{
"key": "key",
"value": "value"
}]
},
"name": "volume_name"
},
"type": "DOCKER_VOLUME"
}
}]
}]
}


Thanks,

Abhishek Dasgupta

Re: Review Request 51870: Added a test for reservation info in the agent's endpoint.

[haosdent huang]

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




src/tests/reservation_endpoints_tests.cpp (lines 1652 - 1654)


I think you `settle` here to wait for `Slave::checkpointResources` 
finished, would it better to add a comment to descirbe why need to settle here?


- haosdent huang


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51870/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-6085
> https://issues.apache.org/jira/browse/MESOS-6085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for reservation info from the agent's endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
> 
> Diff: https://reviews.apache.org/r/51870/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51868: Expose full reservation info in the agent's http endpoint.

[haosdent huang]

> On Sept. 18, 2016, 2:01 p.m., haosdent huang wrote:
> > src/slave/http.cpp, lines 985-997
> > 
> >
> > Should we follow what we do in `master/http.cpp`?
> > 
> > ```
> >   hashmap reserved =
> > slave->totalResources.reservations();
> > 
> >   writer->field(
> >   "reserved_resources_full",
> >   [](JSON::ObjectWriter* writer) {
> > foreachpair (const string& role,
> >  const Resources& resources,
> >  reserved) {
> >   writer->field(role, [](JSON::ArrayWriter* 
> > writer) {
> > foreach (const Resource& resource, resources) {
> >   writer->element(JSON::Protobuf(resource));
> > }
> >   });
> > }
> >   });
> > ```

Saw you have updated to `totalResources.reservations()` in 
https://reviews.apache.org/r/51869/ :)


- haosdent


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


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51868/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose full reservation info in the agent's http endpoint.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
> 
> Diff: https://reviews.apache.org/r/51868/diff/
> 
> 
> Testing
> ---
> 
> make check. A new test is added in https://reviews.apache.org/r/51870/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51868: Expose full reservation info in the agent's http endpoint.

[haosdent huang]

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



The documents `docs/persistent-volume.md` and `docs/reservation.md` need to be 
updated.


src/slave/http.cpp (lines 985 - 997)


Should we follow what we do in `master/http.cpp`?

```
  hashmap reserved =
slave->totalResources.reservations();

  writer->field(
  "reserved_resources_full",
  [](JSON::ObjectWriter* writer) {
foreachpair (const string& role,
 const Resources& resources,
 reserved) {
  writer->field(role, [](JSON::ArrayWriter* 
writer) {
foreach (const Resource& resource, resources) {
  writer->element(JSON::Protobuf(resource));
}
  });
}
  });
```


- haosdent huang


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51868/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose full reservation info in the agent's http endpoint.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
> 
> Diff: https://reviews.apache.org/r/51868/diff/
> 
> 
> Testing
> ---
> 
> make check. A new test is added in https://reviews.apache.org/r/51870/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>