Re: Review Request 43701: Added a command executor based on the new V1 API.

[Shuai Lin]

> On Feb. 19, 2016, 4:24 a.m., Shuai Lin wrote:
> > Maybe we should also add a test that launches a task with this new http cmd 
> > executor?

ping


- Shuai


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> ---
> 
> (Updated Feb. 20, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a command executor based on the new V1 API.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

[Shuai Lin]

> On Feb. 18, 2016, 4:07 p.m., Shuai Lin wrote:
> > src/slave/flags.cpp, line 693
> > 
> >
> > One space before `\n`, otherwise the word would be mixed with the first 
> > word of the next line.
> 
> Qian Zhang wrote:
> I do not think we will have such issue, you can take a look at the all 
> other existing flags, they all do the same way. And here is the output when I 
> run "./src/mesos-slave --help":
> # ./src/mesos-slave --help
> Usage: lt-mesos-slave [options]
> ...
> --[no-]http_command_executor  Enable mesos 
> containerizer to use HTTP command executor which uses
>   executor HTTP API to 
> interact with Mesos agent. If false, the old
>   command executor which 
> uses executor driver API will be used.
>   default: false)
> ...
> 
> I think the output is desired.

You're right. I didn't note there is an explicit `"\n"` in the end of every 
line.


- Shuai


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> ---
> 
> (Updated Feb. 20, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a command executor based on the new V1 API.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

[Shuai Lin]

> On Feb. 18, 2016, 4:07 p.m., Shuai Lin wrote:
> > src/slave/slave.cpp, line 3709
> > 
> >
> > Instead of repeating the `if (flags.http_command_executor)...` logic 
> > multiple times, I would prefer use a temp variable to store either 
> > `mesos-executor` or `mesos-http-executor`.
> 
> Qian Zhang wrote:
> I am not sure if I get your point correctly, can you please clarify how 
> to use the temp var? BTW, such logic is just repeated twice in 
> `Slave::getExecutorInfo`, it seems not a big deal to me :-)

Fair enough.


- Shuai


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> ---
> 
> (Updated Feb. 20, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a command executor based on the new V1 API.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43718: Added fs::supported() function.

[Shuai Lin]

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

(Updated Feb. 21, 2016, 7:46 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added fs::supported() function.


Diffs
-

  src/linux/fs.hpp fddee2441e008dc70d9cff5e63536ef21c865595 
  src/linux/fs.cpp 0df194207a66bf4c417349bebcc8912acbf5976e 
  src/tests/containerizer/fs_tests.cpp 29e43877612fa151e6f6d79268a7411272a7bfeb 

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


Testing (updated)
---

make check on ubuntu 14.04 64bit vm


Thanks,

Shuai Lin

Re: Review Request 43718: Added fs::supported() function.

[Shuai Lin]

> On Feb. 19, 2016, 9:34 a.m., haosdent huang wrote:
> > src/linux/fs.cpp, line 59
> > 
> >
> > `tokens.back()` instead of `tokens[tokens.size() - 1]`?

Good suggestion, updated.


- Shuai


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


On Feb. 21, 2016, 7:43 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43718/
> ---
> 
> (Updated Feb. 21, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4707
> https://issues.apache.org/jira/browse/MESOS-4707
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added fs::supported() function.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp fddee2441e008dc70d9cff5e63536ef21c865595 
>   src/linux/fs.cpp 0df194207a66bf4c417349bebcc8912acbf5976e 
>   src/tests/containerizer/fs_tests.cpp 
> 29e43877612fa151e6f6d79268a7411272a7bfeb 
> 
> Diff: https://reviews.apache.org/r/43718/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 43718: Added fs::supported() function.

[Shuai Lin]

> On Feb. 19, 2016, 9:28 a.m., haosdent huang wrote:
> > src/tests/containerizer/fs_tests.cpp, line 49
> > 
> >
> > I use CentOS 6. But seems don't have `ext2` and `ext3` in 
> > `/proc/filesystems`
> > 
> > ```
> > at /proc/filesystems
> > nodev   sysfs
> > nodev   rootfs
> > nodev   bdev
> > nodev   proc
> > nodev   cgroup
> > nodev   cpuset
> > nodev   tmpfs
> > nodev   devtmpfs
> > nodev   binfmt_misc
> > nodev   debugfs
> > nodev   securityfs
> > nodev   sockfs
> > nodev   usbfs
> > nodev   pipefs
> > nodev   anon_inodefs
> > nodev   inotifyfs
> > nodev   devpts
> > nodev   ramfs
> > nodev   hugetlbfs
> > iso9660
> > nodev   pstore
> > nodev   mqueue
> > ext4
> > vfat
> > ```
> 
> Guangya Liu wrote:
> I think that's why using EXPECT_SOME_TRUE here. The ubuntu does have this.
> 
> haosdent huang wrote:
> Yes, but this test case would fail in CentOS 6?

Good catch, I think that's because ext2 and ext3 modules are not loaded on your 
server by default. I removed "ext2" and "ext3" test, and added "rootfs" test.


- Shuai


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


On Feb. 21, 2016, 7:43 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43718/
> ---
> 
> (Updated Feb. 21, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4707
> https://issues.apache.org/jira/browse/MESOS-4707
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added fs::supported() function.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp fddee2441e008dc70d9cff5e63536ef21c865595 
>   src/linux/fs.cpp 0df194207a66bf4c417349bebcc8912acbf5976e 
>   src/tests/containerizer/fs_tests.cpp 
> 29e43877612fa151e6f6d79268a7411272a7bfeb 
> 
> Diff: https://reviews.apache.org/r/43718/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 43718: Added fs::supported() function.

[Shuai Lin]

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

(Updated Feb. 21, 2016, 7:43 a.m.)


Review request for mesos and Jie Yu.


Changes
---

CentOS 6 doesn't have ext2/ext3 modules loaded by default.


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


Repository: mesos


Description
---

Added fs::supported() function.


Diffs (updated)
-

  src/linux/fs.hpp fddee2441e008dc70d9cff5e63536ef21c865595 
  src/linux/fs.cpp 0df194207a66bf4c417349bebcc8912acbf5976e 
  src/tests/containerizer/fs_tests.cpp 29e43877612fa151e6f6d79268a7411272a7bfeb 

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


Testing
---

make check


Thanks,

Shuai Lin

Re: Review Request 43807: Replaced with `~` in authentication.md.

[Klaus Ma]

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

(Updated Feb. 21, 2016, 2:42 p.m.)


Review request for mesos, Adam B and Till Toenshoff.


Changes
---

Update summary.


Summary (updated)
-

Replaced with `~` in authentication.md.


Repository: mesos


Description (updated)
---

Replaced  with `~` in authentication.md.


Diffs
-

  docs/authentication.md c7649bb279312e3183d8c977811d12121208f9f8 

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


Testing
---

Can not parss `~` to $HOME in Mac OS; update document to use $HOME. Test passed 
in Mac OS.

Failed when using `~`:
I0221 13:08:41.717308 4284416 credentials.hpp:35] Loading credentials for 
authentication from '~/credentials'
Failed to read credentials file '~/credentials': Failed to open file 
'~/credentials': No such file or directory (see --credentials flag)


Thanks,

Klaus Ma

Re: Review Request 43807: Replaced with /Users/klaus in authentication.md.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43807]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 21, 2016, 5:10 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43807/
> ---
> 
> (Updated Feb. 21, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Adam B and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced  with /Users/klaus in authentication.md.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md c7649bb279312e3183d8c977811d12121208f9f8 
> 
> Diff: https://reviews.apache.org/r/43807/diff/
> 
> 
> Testing
> ---
> 
> Can not parss `~` to $HOME in Mac OS; update document to use $HOME. Test 
> passed in Mac OS.
> 
> Failed when using `~`:
> I0221 13:08:41.717308 4284416 credentials.hpp:35] Loading credentials for 
> authentication from '~/credentials'
> Failed to read credentials file '~/credentials': Failed to open file 
> '~/credentials': No such file or directory (see --credentials flag)
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Review Request 43807: Replaced with /Users/klaus in authentication.md.

[Klaus Ma]

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

Review request for mesos, Adam B and Till Toenshoff.


Repository: mesos


Description
---

Replaced  with /Users/klaus in authentication.md.


Diffs
-

  docs/authentication.md c7649bb279312e3183d8c977811d12121208f9f8 

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


Testing
---

Can not parss `~` to $HOME in Mac OS; update document to use $HOME. Test passed 
in Mac OS.

Failed when using `~`:
I0221 13:08:41.717308 4284416 credentials.hpp:35] Loading credentials for 
authentication from '~/credentials'
Failed to read credentials file '~/credentials': Failed to open file 
'~/credentials': No such file or directory (see --credentials flag)


Thanks,

Klaus Ma

Re: Review Request 43806: Add comments for rebalance.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43806]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 21, 2016, 2:26 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43806/
> ---
> 
> (Updated Feb. 21, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-3943
> https://issues.apache.org/jira/browse/MESOS-3943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add comments for rebalance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
> 
> Diff: https://reviews.apache.org/r/43806/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41597: Extending allocator interface to support dynamic weights.

[Yongqiao Wang]

> On Feb. 19, 2016, 9:36 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1068-1070
> > 
> >
> > Why don't we need to set `rebalance` here?
> 
> Yongqiao Wang wrote:
> According to my understanding, A role appears in `quotaRoleSorter` if it 
> has a quota but it does not means it has one or more registered frameworks, 
> so if there is no framework register to this role, it does not need to 
> trigger the allocation immediately after changing it's weight.
> 
> Alexander Rukletsov wrote:
> This is correct, there may be no frameworks in a quota'd role. However, 
> there may also be some. Currently they are also added to `roleSorter`, but it 
> may change in the future. Can we at least add a comment explaining why we are 
> not rebalancing for the quota sorter?

OK, I have posted a new patch for adding this comments:  
https://reviews.apache.org/r/43806/


- Yongqiao


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


On Feb. 14, 2016, 11:37 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41597/
> ---
> 
> (Updated Feb. 14, 2016, 11:37 a.m.)
> 
> 
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
> 
> 
> Bugs: MESOS-3943
> https://issues.apache.org/jira/browse/MESOS-3943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extending allocator interface to support dynamic weights.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp e163669c9c4e4c98572968f18987704b60722a79 
>   include/mesos/mesos.proto 0bd5abadb5abe052161963ca995c396f1ed832f2 
>   include/mesos/v1/mesos.proto 38e04cb19e303d1c71d2afad6ea73137aaa7403a 
>   src/master/allocator/mesos/allocator.hpp 
> 581eaad376e7b2febe0b6359014617b935a677a3 
>   src/master/allocator/mesos/hierarchical.hpp 
> 20d7ceb1a75ea5cb9efb1fc7fb085265b32864fe 
>   src/master/allocator/mesos/hierarchical.cpp 
> a9d2c23162892e0f97d89a076d2311091d91 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 4669149b81de39b4bb921ef7cd6787aa583f6e40 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 18797e42a9c2bd20392020237cfae600a5ffe12c 
>   src/master/allocator/sorter/sorter.hpp 
> a0a779b81f6d048271f15256b38ff907ae144b83 
>   src/tests/allocator.hpp 206e9ac3a83038a691f7929bdd627042b0f363b0 
> 
> Diff: https://reviews.apache.org/r/41597/diff/
> 
> 
> Testing
> ---
> 
> Make & Make check successfully!
> 
> Test case: https://reviews.apache.org/r/41672/
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Review Request 43806: Add comments for rebalance.

[Yongqiao Wang]

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

Review request for mesos, Adam B and Alexander Rukletsov.


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


Repository: mesos


Description
---

Add comments for rebalance.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 

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


Testing
---


Thanks,

Yongqiao Wang

Re: Review Request 43802: Wrapped TASK_LOST with `` in authorization.md.

[Klaus Ma]

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

(Updated Feb. 21, 2016, 9:05 a.m.)


Review request for mesos and Adam B.


Changes
---

Update summary to end with period.


Summary (updated)
-

Wrapped TASK_LOST with `` in authorization.md.


Repository: mesos


Description
---

Wrapped TASK_LOST with `` in authorization.md


Diffs
-

  docs/authorization.md bbb4f2a 

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


Testing
---


Thanks,

Klaus Ma

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

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

(Updated Feb. 20, 2016, 6:47 p.m.)


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


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


Repository: mesos


Description
---

Fix for docker containerizer not configuring CFS quotas correctly.

It would be nice to refactor all this isolation code in a way that can be 
shared between all containerizers, as this is basically just copied from the 
CgroupsCpushareIsolator, but that's a much bigger undertaking.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 

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


Testing
---


Thanks,

Steve Niemitz

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 811-815
> > 
> >
> > This is done after the container has been launched. We should 
> > definitely call out the fact that there's a short window after the 
> > container has been launched before the container's cfs quota is updated. 
> > During that window, it's not subject to cfs throttling.
> > 
> > Alternatively, we could set `--cpu-period` and `--cpu-quota` during 
> > launch so that we don't have to worry about that window.
> 
> Steve Niemitz wrote:
> Is there a minimum supported version of docker for mesos?  Those 
> parameters were added in docker 1.7 I believe.
> 
> Jie Yu wrote:
> Yeah, I think for now, add a TODO should be sufficient. Once we bump the 
> minimum docker version support, we can address that TODO.

sgtm


- Steve


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


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 43450: Fixed typo in log message.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On Feb. 10, 2016, 11:16 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43450/
> ---
> 
> (Updated Feb. 10, 2016, 11:16 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the `uuid` field of `StatusUpdate` is not set, the output of
> `operator<<(ostream& stream, const StatusUpdate& update)` contains a
> mismatched parenthesis. In practice, this means we can omit log
> messages that have a typo.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.cpp 89deee34948bcf0fadcabc0fbdd4356c0bd2ddb7 
> 
> Diff: https://reviews.apache.org/r/43450/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Old log output:
> 
> ```
> Sending status update TASK_LOST) for task 
> 33fa8ceb-f133-43c0-8a8d-9a77f83b825b of framework 
> 06daa081-8522-42b1-a4d3-1160c1f0662d- 'Task launched with invalid offers: 
> Offer 06daa081-8522-42b1-a4d3-1160c1f0662d-O1 is no longer valid'
> ```
> 
> New log output:
> 
> ```
> Sending status update TASK_LOST for task f3674b8a-5242-4864-8bab-cbb5f342de05 
> of framework 5bc9cf4c-4767-4800-92e1-b6b56d17edd3- 'Task launched with 
> invalid offers: Offer 5bc9cf4c-4767-4800-92e1-b6b56d17edd3-O1 is no longer 
> valid'
> ```
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, line 785
> > 
> >
> > Do you need to call update for this case (command task but agent is not 
> > running in a docker container)?
> 
> Steve Niemitz wrote:
> The command executor is a totally different code path, so calling update 
> wouldn't update the right thing.  I'd like to handle that in a seperate 
> review since its more complicated.
> 
> Jie Yu wrote:
> Hum, why do you think it's a totally different code path? 'update' will 
> identify the correct container (the container launched by the command 
> executor) by calling docker inspect using container->name(). The way we 
> generate container name is a little implicit in the code. I think you can 
> just do an 'update' after launch the executor process similar to the custom 
> executor's case. Or am I missing something?

Yeah you're totally right!  Thanks for clarifying that.  I'll make the changes 
here too.  That's awesome this should work for both code paths now.


- Steve


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


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 43382: Updated Doxyfile to use relative path.

[Ben Mahler]

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


Ship it!




Thanks for clarifying! I see what this is doing now.

- Ben Mahler


On Feb. 20, 2016, 4:04 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43382/
> ---
> 
> (Updated Feb. 20, 2016, 4:04 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By default, doxygen generated html pages use absolute path for links
> (due to FULL_PATH_NAMES flag set to 'yes').  Thus the links break when
> these pages are moved to a different directory. The fix is to ask
> doxygen to strip out '/path/to/mesos/' from the generated links.
> 
> 
> Diffs
> -
> 
>   Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15 
> 
> Diff: https://reviews.apache.org/r/43382/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 43802: Wrapped TASK_LOST with `` in authorization.md

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [43802]

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

Error:
2016-02-20 16:40:47 URL:https://reviews.apache.org/r/43802/diff/raw/ 
[1483/1483] -> "43802.patch" [1]
No files to lint

Error: Commit message summary (the first line) must end in a period.

Full log: https://builds.apache.org/job/mesos-reviewbot/11559/console

- Mesos ReviewBot


On Feb. 20, 2016, 4:36 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43802/
> ---
> 
> (Updated Feb. 20, 2016, 4:36 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped TASK_LOST with `` in authorization.md
> 
> 
> Diffs
> -
> 
>   docs/authorization.md bbb4f2a 
> 
> Diff: https://reviews.apache.org/r/43802/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Review Request 43802: Wrapped TASK_LOST with `` in authorization.md

[Klaus Ma]

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

Review request for mesos and Adam B.


Repository: mesos


Description
---

Wrapped TASK_LOST with `` in authorization.md


Diffs
-

  docs/authorization.md bbb4f2a 

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


Testing
---


Thanks,

Klaus Ma

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Ben Mahler]

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



It would be helpful to reference in the testing done section that you've also 
added tests in subsequent patches, so that it's clear to the reviewer without 
having to look through the entire chain. I left some comments but I'll make the 
adjustments since they're minor.


src/docker/executor.cpp (lines 209 - 212)


Ditto here, I can't see why this is only done inside killTask, looks like a 
bug!



src/launcher/executor.cpp (line 117)


To be consistent, let's set frameworkInfo right below where we set driver.



src/launcher/executor.cpp (lines 460 - 469)


Let's do this inside shutdown per my comment at the top.

It's also not clear to me why the health check process is only killed 
inside killTask, seems that we'll leak the health check process if we only are 
sent shutdown! Looks like a bug :(



src/launcher/executor.cpp (lines 460 - 461)


Let's avoid copying every capability we loop over (take a const & instead), 
since Capability may store additional data in the future and we don't need to 
mutate a copy here.



src/launcher/executor.cpp (line 461)


Please add a CHECK_SOME to ensure that frameworkInfo is Some before you 
de-reference.

Also, you can use the -> operator to clean this up:

```
foreach (FrameworkInfo::Capability capability,
 frameworkInfo->capabilities()) {
```

A space is needed before the brace on the foreach: s/){/) {/



src/launcher/executor.cpp (line 464)


Please use CopyFrom for copying protobufs, unless you need the merge 
functionality (you don't need merging functionality here).


- Ben Mahler


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 40748: Updated documentation to point out the need of a resolvable hostname.

[Ben Mahler]

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




docs/getting-started.md (line 26)


It would also be great to print out a helpful diagnostic message when 
someone is running the tests (or the agent) and we don't find the right 
configuration needed for the docker containerizer to function correctly.

I just spent 15 minutes tracking down why the test was failing for me, and 
had to read through the lengthy conversation in 
[MESOS-3937](https://issues.apache.org/jira/browse/MESOS-3937) before seeing 
that I needed to adjust my hostname in vagrant, but a diagnostic message would 
have alerted me immediately to the issue.


- Ben Mahler


On Nov. 26, 2015, 1:08 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40748/
> ---
> 
> (Updated Nov. 26, 2015, 1:08 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Lukas Loesche, and Timothy Chen.
> 
> 
> Bugs: MESOS-3937
> https://issues.apache.org/jira/browse/MESOS-3937
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md d120ad42d6e783048e6261750a9e7ad650d03669 
> 
> Diff: https://reviews.apache.org/r/40748/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 43319: Remove markdown files from doxygen.

[Kapil Arya]

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

(Updated Feb. 20, 2016, 11:10 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

addressed BenM's comments.


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


Repository: mesos


Description
---

* The doxygen html pages corresponding to doc/* markdown files are
  redundant and have broken links. As such, they don't serve any
  reasonable purpose in doxygen site.
* Also updated the main page to include direct links to libprocess/stout
  developer guides.
* Updated site/README.md to include instructions for updating doxygen
  and javadocs pages.


Diffs (updated)
-

  Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15 
  site/README.md 78609fb68182338199b4d0ad4c333c8f34274ee5 
  src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1 

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 43382: Updated Doxyfile to use relative path.

[Kapil Arya]

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

(Updated Feb. 20, 2016, 11:04 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

added a comment.


Repository: mesos


Description (updated)
---

By default, doxygen generated html pages use absolute path for links
(due to FULL_PATH_NAMES flag set to 'yes').  Thus the links break when
these pages are moved to a different directory. The fix is to ask
doxygen to strip out '/path/to/mesos/' from the generated links.


Diffs (updated)
-

  Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15 

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 43701: Added a command executor based on the new V1 API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43701]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> ---
> 
> (Updated Feb. 20, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a command executor based on the new V1 API.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43490: Added testcase for TASK_KILLING state.

[Ben Mahler]

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



Hm.. I would expect this patch to be testing the functionality that we've 
introduced within the command and docker executors, since that's where the 
logic resides. This does not test the functionality added by the previous 
patch, why bother mocking out the logic instead of just testing the command and 
docker executors?

- Ben Mahler


On Feb. 16, 2016, 6:23 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43490/
> ---
> 
> (Updated Feb. 16, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added testcase for TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c0997dbacf8753149caba0a8b2406afc35a4845f 
>   src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 
> 
> Diff: https://reviews.apache.org/r/43490/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

[Qian Zhang]

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

(Updated Feb. 20, 2016, 9:44 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Added a command executor based on the new V1 API.


Diffs (updated)
-

  docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
  include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
  src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
  src/launcher/http_executor.cpp PRE-CREATION 
  src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
  src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
  src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 43487: Add TASK_KILLING state.

[Ben Mahler]

> On Feb. 20, 2016, 11:55 a.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, line 1109
> > 
> >
> > Let's clarify that "being killed" in this context is referring to the 
> > executor having received the kill request, for example:
> > 
> > // The task is being killed by the executor, but is not yet killed.

Let's also reference the TASK_KILLING_STATE capability here so that it's clear 
in the API that this requires the framework to set a capability.


- Ben


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


On Feb. 16, 2016, 9:51 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43487/
> ---
> 
> (Updated Feb. 16, 2016, 9:51 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 
>   include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116 
>   src/master/http.cpp 3d7a624b78fd85a8d99bce609e37411ed660678c 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp e1ca81dab85a7ab1391eca0d6bd995548bf79c22 
>   src/master/metrics.hpp 551e4eb0f910aa4ce28a2a955019108fcd3d6371 
>   src/master/metrics.cpp 5e4f4d352579dcd391a133600f9d4abc6b2402a7 
>   src/slave/metrics.hpp 2c22de521309806d70f9bd124359756b5e4c75b4 
>   src/slave/metrics.cpp 07694955d21efc2509e0eebcd0b5957249d13319 
>   src/slave/slave.hpp ced835dec797bcc5640422468487a4289a737e38 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
>   src/tests/metrics_tests.cpp 106bea58b0714ae745df73597c702e4815523938 
> 
> Diff: https://reviews.apache.org/r/43487/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Ben Mahler]

> On Feb. 18, 2016, 1:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??
> 
> Abhishek Dasgupta wrote:
> Actually, to set the status, we need to send the task_id also. Task_id is 
> not available inside shutdown. Moreover, I don't think it will make any 
> difference if it is called inside shutdown instead of killTask. It makes 
> sense to change the status a TASK_KILLING as soon as killTask is issued.
> 
> Qian Zhang wrote:
> shutdown() will be not only called by killTask(), but also called when 
> the executor itself is asked to shutdown, so if you send the status in 
> killTask(), that means for the latter case (executor shutdown), the 
> TASK_KILLING status will not be sent?
> 
> Abhishek Dasgupta wrote:
> So, here is the dubious scenario. If executor shuts down, you can't 
> exepect to provide task_id to shutdown() as there might be lots of tasks. 
> That's why the only argument of shutdown() is executordriver. But to update 
> TASK_KILLING status, you must need task_id. So, in my understanding, shutdown 
> by executor shut down should not require TASK_KILLING status as in that case, 
> all the tasks are unhealthy.

Recall that executors can manage many tasks, the command/docker executors are 
implementations provided by mesos and they manage a single task only. It is up 
to the executor to decide how to shutdown, in the case of the command executor 
we kill the active task and report TASK_KILLED (in both the killTask and 
shutdown cases):

https://github.com/apache/mesos/blob/0.27.0/src/launcher/executor.cpp#L514-L516

Since we'd like to send a TASK_KILLING when we start to kill a task, we need to 
send it in both the killTask and shutdown.

Make sense?


- Ben


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


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43488: Adding framework capability for TASK_KILLING.

[Ben Mahler]

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


Fix it, then Ship it!




Thanks! I'll make the adjustments here since they're minor.


include/mesos/mesos.proto (line 261)


Newline here. How about the following:

```
  // Receive the TASK_KILLING TaskState when a task is being
  // killed by an executor. The executor will examine this
  // capability to determine whether it can send TASK_KILLING.
  TASK_KILLING_STATE = 2;
```



src/tests/master_tests.cpp (lines 3019 - 3024)


We would typically stick a newline in front of the 'capabilityType' 
variable since there is an expression above it.

How about 'capabilities' as a name? Note that this is a plural name which 
indicates to the reader that this is a container rather than a single 
capability.

How about using an initializer list here? Altogether:

```
  FrameworkInfo framework = DEFAULT_FRAMEWORK_INFO;
  framework.set_webui_url("http://localhost:8080/;);

  vector capabilities = {
FrameworkInfo::Capability::REVOCABLE_RESOURCES,
FrameworkInfo::Capability::TASK_KILLING_STATE
  };

  foreach (FrameworkInfo::Capability::Type capability, capabilities) {
framework.add_capabilities()->set_type(capability);
  }
```



src/tests/master_tests.cpp (lines 3060 - 3069)


There are a few places where you're using 'as' here directly without first 
asserting that it 'is' the right type, so the test can crash if the assumptions 
don't hold. We'd rather have assertions fail than have the entire test suite 
crash so it would be great to check 'is' before using 'as':

```
  EXPECT_EQ(1u, framework_.values.count("capabilities"));
  ASSERT_TRUE(framework_.values["capabilities"].is());

  vector actual;

  foreach (const JSON::Value& capability,
   framework_.values["capabilities"].as().values) {
FrameworkInfo::Capability::Type type;

ASSERT_TRUE(capability.is());
ASSERT_TRUE(
FrameworkInfo::Capability::Type_Parse(
capability.as().value,
));

actual.push_back(type);
  }

  EXPECT_EQ(capabilities, actual);
```

I'll make sure to update this test so that there isn't 'as' usage without 
an assertion on 'is'.


- Ben Mahler


On Feb. 16, 2016, 3:30 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43488/
> ---
> 
> (Updated Feb. 16, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding framework capability for TASK_KILLING.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 
>   include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116 
>   src/tests/master_tests.cpp 393a6f5fe3744d6ba743f362b7e309d1ee75a303 
> 
> Diff: https://reviews.apache.org/r/43488/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43487: Add TASK_KILLING state.

[Ben Mahler]

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


Ship it!




Thanks! I left a minor comment but I'll take care of it.


include/mesos/mesos.proto (line 1109)


Let's clarify that "being killed" in this context is referring to the 
executor having received the kill request, for example:

// The task is being killed by the executor, but is not yet killed.


- Ben Mahler


On Feb. 16, 2016, 9:51 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43487/
> ---
> 
> (Updated Feb. 16, 2016, 9:51 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 
>   include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116 
>   src/master/http.cpp 3d7a624b78fd85a8d99bce609e37411ed660678c 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp e1ca81dab85a7ab1391eca0d6bd995548bf79c22 
>   src/master/metrics.hpp 551e4eb0f910aa4ce28a2a955019108fcd3d6371 
>   src/master/metrics.cpp 5e4f4d352579dcd391a133600f9d4abc6b2402a7 
>   src/slave/metrics.hpp 2c22de521309806d70f9bd124359756b5e4c75b4 
>   src/slave/metrics.cpp 07694955d21efc2509e0eebcd0b5957249d13319 
>   src/slave/slave.hpp ced835dec797bcc5640422468487a4289a737e38 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
>   src/tests/metrics_tests.cpp 106bea58b0714ae745df73597c702e4815523938 
> 
> Diff: https://reviews.apache.org/r/43487/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43800: Updated authorization docs for '/reserve' and '/create-volumes'.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43776, 43777, 43782, 43778, 43779, 43800]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 20, 2016, 1:32 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43800/
> ---
> 
> (Updated Feb. 20, 2016, 1:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated authorization docs for '/reserve' and '/create-volumes'.
> 
> This updates the authorization documentation to include the new `roles` 
> object for the `CreateVolume` and `ReserveResources` ACLs.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md bbb4f2adc9348cb1686e6af78f5604d8cf7651ab 
> 
> Diff: https://reviews.apache.org/r/43800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43319: Remove markdown files from doxygen.

[Ben Mahler]

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


Fix it, then Ship it!





site/README.md (lines 24 - 25)


Seems like this blurb belongs in the release guide rather than the site 
readme?


- Ben Mahler


On Feb. 9, 2016, 11:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43319/
> ---
> 
> (Updated Feb. 9, 2016, 11:22 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4619
> https://issues.apache.org/jira/browse/MESOS-4619
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * The doxygen html pages corresponding to doc/* markdown files are
>   redundant and have broken links. As such, they don't serve any
>   reasonable purpose in doxygen site.
> * Also updated the main page to include direct links to libprocess/stout
>   developer guides.
> * Updated site/README.md to include instructions for updating doxygen
>   and javadocs pages.
> 
> 
> Diffs
> -
> 
>   Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15 
>   site/README.md 78609fb68182338199b4d0ad4c333c8f34274ee5 
>   src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1 
> 
> Diff: https://reviews.apache.org/r/43319/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 43382: Updated Doxyfile to use relative path.

[Ben Mahler]

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



Hm.. I still don't follow from the description what this change is doing.

The testing done section is empty, I assume you've tested this somehow?


Doxyfile (lines 110 - 117)


Can you document for posterity why we set this? I'm having trouble 
following why this change is being done.


- Ben Mahler


On Feb. 10, 2016, 12:16 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43382/
> ---
> 
> (Updated Feb. 10, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By default, doxygen generated html pages use absolute path for links. Thus te 
> links break when these pages are moved to a different directory.
> 
> 
> Diffs
> -
> 
>   Doxyfile 1b0a2beda1d9223eafd9d14d335c651eb4961a15 
> 
> Diff: https://reviews.apache.org/r/43382/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 43800: Updated authorization docs for '/reserve' and '/create-volumes'.

[Guangya Liu]

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




docs/authorization.md (lines 215 - 244)


Can you please also add an example of `reserve_resources` with some 
specified roles?



docs/authorization.md (line 285)


What about adding another two examples for `create_volumes` with `"type" : 
"None"` and specified `roles`?


- Guangya Liu


On 二月 20, 2016, 1:32 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43800/
> ---
> 
> (Updated 二月 20, 2016, 1:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated authorization docs for '/reserve' and '/create-volumes'.
> 
> This updates the authorization documentation to include the new `roles` 
> object for the `CreateVolume` and `ReserveResources` ACLs.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md bbb4f2adc9348cb1686e6af78f5604d8cf7651ab 
> 
> Diff: https://reviews.apache.org/r/43800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43779: Added '/reserve' tests with multiple roles.

[Guangya Liu]

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




src/tests/reservation_endpoints_tests.cpp (line 1035)


s/ Reserve/Unreserve / `ReserveResources`



src/tests/reservation_endpoints_tests.cpp (line 1036)


s/ reserve/unreserve / reserve


- Guangya Liu


On 二月 20, 2016, 1:10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43779/
> ---
> 
> (Updated 二月 20, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added '/reserve' tests with multiple roles.
> 
> Operators may reserve resources for multiple roles in the same operation; 
> this patch adds tests to confirm correct behavior of authorization in this 
> case. The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and 
> `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> afe81b1d38a1b3a82583720f26482ddcde8f5e85 
> 
> Diff: https://reviews.apache.org/r/43779/diff/
> 
> 
> Testing
> ---
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43801: Refactored and simplified the docker puller interfaces.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43801]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 20, 2016, 6:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43801/
> ---
> 
> (Updated Feb. 20, 2016, 6:09 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Bugs: MESOS-4499
> https://issues.apache.org/jira/browse/MESOS-4499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the first step of cleaning up the docker registry puller/client code 
> base. I did the following simplications:
> 
> 1) Simplify the paths functions. The getImageArchiveXXX functions are 
> redundant.
> 2) Simplify the Puller::pull interface. We just need to return layer ids and 
> no need to return path.
> 3) Used untar from command utils, and kill the same function in puller.cpp.
> 4) A lot of consistency fixes, including using the same type and name.
> 5) A lot of style fixes.
> 
> The next step will be to cleanup and refactor registry_puller to use uri 
> Fetcher (docker plugin) to download manifest and blobs (and kill registry 
> client).
> 
> Reviewers are recommended to look at the end version, instead of the diff.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 811c24b43f7aec9db406dd521770c6cd82097c92 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f3e7c042f2c9980f6274c8185ee8bb89a8b02002 
>   src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
> d2b0cf95e5ed7664071484a27a2a1f9bafff70d6 
>   src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
> 82d92a2a9cce181eb283395a32e93cbb1586703b 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 5b2d72c22fcbcc379b4901607cf3eb682de66206 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> a239b97557ad20353c67050dbc89ef16da898330 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
> bccbac3357cf942446604e6cf5d16c3d594b 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 3fcf1471a035e35a2cac22442655ad65a84a9793 
>   src/slave/containerizer/mesos/provisioner/docker/store.hpp 
> 880e216e33dd178d0baa47d3958c84cda4d9e25e 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 2f1d3e002140f34c646aab445a419c9c3d712f99 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 4db6793a21abbb7ea4de0d0fca0431237d38d013 
> 
> Diff: https://reviews.apache.org/r/43801/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 43782: Changed object of `CreateVolume` ACL to `roles`.

[Guangya Liu]

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




src/tests/authorization_tests.cpp (line 565)


s/can/can only



src/tests/authorization_tests.cpp (lines 575 - 576)


Do we still need ` and principal "bar" will not be  allowed to create 
volumes for any other roles.` if update line 567 to `Principal "bar" can only 
create volumes for the "panda" role.`

Or else move this comment to line 567?


- Guangya Liu


On 二月 20, 2016, 1:08 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43782/
> ---
> 
> (Updated 二月 20, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed object of `CreateVolume` ACL to `roles`.
> 
> This solves a problem in which any principal could create volumes for any 
> role using the '/create-volumes' operator endpoint.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 
>   src/authorizer/local/authorizer.cpp 
> 9557bbdf68ff182c4538bbf70cee576d717abc05 
>   src/master/master.cpp e5aaf67e63996700b2cdcdd04055ad5b04bfb085 
>   src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6069ca1e9ed278459c5182e438417e95955b1924 
>   src/tests/persistent_volume_tests.cpp 
> e169e1b141a38dc389eefd42c11a078c413123d5 
> 
> Diff: https://reviews.apache.org/r/43782/diff/
> 
> 
> Testing
> ---
> 
> Persistent volume tests and operation validation tests were altered to 
> accomodate the new ACL object, and some new principal/role combinations were 
> added to `AuthorizationTest.CreateVolume`.
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43776: Changed object of `ReserveResources` ACL to `roles`.

[Guangya Liu]

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




include/mesos/authorizer/authorizer.proto (line 87)


s/may/can?

Or else 

// Objects: The principal(s) can reserve resources for these roles.

I prefer the latter one which might be more clear and also consistent with 
the comments for `CreateVolume`.



src/tests/authorization_tests.cpp (line 419)


s/can reserve/can only reserve resources



src/tests/authorization_tests.cpp (line 424)


Why adding `and principal "baz" will not be allowed to reserve for roles 
other than "ads".` here?

I think that updating the comments for `acl2` to `Principal "baz" can only 
reserve resources for the "ads" role.`



src/tests/authorization_tests.cpp (line 452)


s/reserve/reserve resources



src/tests/master_validation_tests.cpp (lines 236 - 238)


I think that we need to clarify that the `role` checking except "*" will be 
checked in `authorize`, the validation will not check roles except "*" now.

Otherwise, someone might confused that why a framework with `roleA` can 
reserve resoures for `roleB`?



src/tests/reservation_tests.cpp (line 1338)


not yours, but do you mind update this:

s/This princial/The `DEFAULT_CREDENTIAL` principal



src/tests/reservation_tests.cpp (line 1343)


ditto


- Guangya Liu


On 二月 20, 2016, 1:11 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43776/
> ---
> 
> (Updated 二月 20, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed object of the `ReserveResources` ACL to `roles`.
> 
> This solves a problem in which any principal could reserve resources for any 
> role using the '/reserve' operator endpoint. A new test, 
> `ReserveOperationValidationTest.DisallowReserveForStarRole`, was added.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 
>   src/authorizer/local/authorizer.cpp 
> 9557bbdf68ff182c4538bbf70cee576d717abc05 
>   src/master/master.cpp e5aaf67e63996700b2cdcdd04055ad5b04bfb085 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc 
>   src/tests/master_validation_tests.cpp 
> 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 
>   src/tests/reservation_endpoints_tests.cpp 
> afe81b1d38a1b3a82583720f26482ddcde8f5e85 
>   src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7 
> 
> Diff: https://reviews.apache.org/r/43776/diff/
> 
> 
> Testing
> ---
> 
> Tests were altered to accomodate the new ACL object, and the test 
> `ReserveOperationValidationTest.DisallowReserveForStarRole` was added.
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>