Re: Review Request 53693: Added Stephen Hankinson to contributors list.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On Nov. 12, 2016, 5:43 p.m., Stephen Hankinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53693/
> ---
> 
> (Updated Nov. 12, 2016, 5:43 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Stephen Hankinson to contributors list.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml c1e720eda052af55abf3ee58cacc4ee68b65d7d4 
> 
> Diff: https://reviews.apache.org/r/53693/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Stephen Hankinson
> 
>

Review Request 53693: Added Stephen Hankinson to contributors list.

[Stephen Hankinson]

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

Review request for mesos.


Repository: mesos


Description
---

Added Stephen Hankinson to contributors list.


Diffs
-

  docs/contributors.yaml c1e720eda052af55abf3ee58cacc4ee68b65d7d4 

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


Testing
---


Thanks,

Stephen Hankinson

Review Request 53692: Adding myself as a contributor.

[Stephen Hankinson]

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

Review request for mesos.


Repository: mesos


Description
---

Adding myself as a contributor.


Diffs
-

  docs/contributors.yaml c1e720eda052af55abf3ee58cacc4ee68b65d7d4 

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


Testing
---


Thanks,

Stephen Hankinson

Re: Review Request 50127: Added DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch.

[Guangya Liu]

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




src/tests/containerizer/docker_containerizer_tests.cpp (line 4102)


s/1/one



src/tests/containerizer/docker_containerizer_tests.cpp (line 4192)


s/priviledged/priviledged mode



src/tests/containerizer/docker_containerizer_tests.cpp (lines 4226 - 4229)


```
// We use `docker inspect` to verify that all injected nvidia devices
// are presented inside the running docker container. This includes
// both the control devices (e.g. `/dev/nvidia-uvm`) as well as the GPU
// device itself (i.e. `/dev/nvidia0`).
```


- Guangya Liu


On 十一月 10, 2016, 8:15 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50127/
> ---
> 
> (Updated 十一月 10, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This added a testing case for end-to-end GPU support for docker
> containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> c478d56ffd734b26fd2dbd7bb1ca02ce929f3f16 
> 
> Diff: https://reviews.apache.org/r/50127/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch" make -j 
> check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53533: Extended docker flags to pass devices to mesos-docker-executor.

[Guangya Liu]

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



Summary
```
Extended `dockerFlags()` to pass `devices` to mesos-docker-executor.
```

Description
```
This patch extended `dockerFlags()` to pass `devices` to
mesos-docker-executor, and then the `devices` can be used
by the docker container when mesos-docker-executor start
the docker container.
```

- Guangya Liu


On 十一月 10, 2016, 8:15 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53533/
> ---
> 
> (Updated 十一月 10, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended a new docker flag in docker containerizer to pass devices
> to mesos-docker-executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
> 
> Diff: https://reviews.apache.org/r/53533/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50125: Added mesos-docker-executor support for devices control.

[Guangya Liu]

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



Summary
```
Added '--devices' flag to mesos-docker-executor.
```

Description
```
Added a new flag '--devices' to mesos-docker-executor which is used to
control the feature of devices exposition, isolation and access permission
for docker container.
```


src/docker/executor.cpp (line 801)


kill this


- Guangya Liu


On 十一月 10, 2016, 8:15 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50125/
> ---
> 
> (Updated 十一月 10, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new flag '--devices' to mesos-docker-executor, and gave its
> feature to control devices exposition, isolation, and access permission.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
>   src/docker/executor.cpp eefbc0c2936eb93044c2d8a1b50155171f398562 
> 
> Diff: https://reviews.apache.org/r/50125/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53532: Added parse helper function to 'Docker::Device'.

[Guangya Liu]

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



Summary:
```
Added `parse` helper function for `Docker::Device`.
```

Description:
```
Added a new helper function to parse a string to 'Docker::Device'
'Docker::Device' structure. The string should be in the format of
`PathInHost:PathInContainer:Permission`.
```


src/docker/docker.hpp (line 71)


How about adding a comment here?

```
// Parse a string to docker device, the string should be 
// in the format of `PathInHost:PathInContainer:Permission`.
```


- Guangya Liu


On 十一月 10, 2016, 8:14 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53532/
> ---
> 
> (Updated 十一月 10, 2016, 8:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to parse string input to 'Docker::Device'
> structure.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
> 
> Diff: https://reviews.apache.org/r/53532/diff/
> 
> 
> Testing
> ---
> 
> make -j4 check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50947: Removed isolator flag 'gpu/nvidia' for docker containerizer using GPU.

[Guangya Liu]

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



We do not have `external containerizer` now, so you can remove it from the 
`Descripiton`.

Summary
```
Only check `gpu/nvidia` isolator for mesos containerizer.
```

Description
```
Mesos containerizer uses isolator `gpu/nvidia` for GPU isolation while
docker containerizers do not need it. This patch is making the isolator
`gpu/nvidia` check only available for mesos containerizer but not docker
containerizer in GPU allocator.
```

- Guangya Liu


On 十一月 10, 2016, 8:13 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50947/
> ---
> 
> (Updated 十一月 10, 2016, 8:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5975
> https://issues.apache.org/jira/browse/MESOS-5975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos containerizer uses isolator 'gpu/nvidia' for GPU isolation while
> docker and external containerizers do not need it anymore. This removed
> the 'gpu/nvidia' isolator check for docker and external containerizers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> 2e722691475c84afae14009014ea70cc0fdd0e65 
> 
> Diff: https://reviews.apache.org/r/50947/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 52735: Updated comment message for docker killing.

[Guangya Liu]

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



What about update the summary and description as following? 

Summary:
```
Updated comments for killing some garbage containers.
```

Description:
```
We have already enabled `Garbage collector` for some garbage
containers (failed to be killed when destroy), so the related
comments should be updated to reflect this.
```


src/slave/containerizer/docker.cpp (lines 2176 - 2179)


I think this should be right before #2167


- Guangya Liu


On 十一月 10, 2016, 8:13 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52735/
> ---
> 
> (Updated 十一月 10, 2016, 8:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Garbage collector` has already enabled so that the docker failed
> in killing will be force removed by calling `Self::remove`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
> 
> Diff: https://reviews.apache.org/r/52735/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53691: Implemented some quota functionality tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52284, 53679, 53691]

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 Nov. 12, 2016, 7:59 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53691/
> ---
> 
> (Updated Nov. 12, 2016, 7:59 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
> 
> 
> Bugs: MESOS-4292
> https://issues.apache.org/jira/browse/MESOS-4292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This implements some tests in previous todos for
> quota functionality.
> 
> The one for:
> 
> `Role quota is below its allocation (InverseOffer generation).`
> 
> does not seem be implementable right now, since
> hierarchical allocator does not send InverseOffer for
> quota yet.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
> 
> Diff: https://reviews.apache.org/r/53691/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52103: Implement quota update through `PUT` method.

[Zhitao Li]

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




src/master/master.hpp (lines 1028 - 1032)


It's happening in a follow up patch. I didn't include it here because it's 
not technically part of the PUT change.


- Zhitao Li


On Oct. 19, 2016, 11:28 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52103/
> ---
> 
> (Updated Oct. 19, 2016, 11:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
> 
> 
> Bugs: MESOS-4941
> https://issues.apache.org/jira/browse/MESOS-4941
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement quota update through `PUT` method.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
>   src/master/http.cpp 9005e7c308d5f57c6f5c573951d468a3ba730740 
>   src/master/master.hpp 35db198748b8652eb53e17f592f6b40d1e6a3ed9 
>   src/master/quota_handler.cpp bf6a613a7bb3c62fd77d1ffde3170749d6c21fa2 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
> 
> Diff: https://reviews.apache.org/r/52103/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52103: Implement quota update through `PUT` method.

[Zhitao Li]

> On Oct. 20, 2016, 3:53 p.m., Alexander Rukletsov wrote:
> > What about updating "quota.md"? How a quota update request will look like 
> > for an operator? I assume the same as quota set, but we should call it out 
> > in the docs.

Doc update and operator API implementation will sent with a follow up patch.


> On Oct. 20, 2016, 3:53 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1140-1143
> > 
> >
> > Even if we leave one function in the allocator for setting *and* 
> > updating quota (I'm not convinced yet) we should, don't you think it is 
> > valuable to leave most of this comment so people understand why there are 
> > two separate paths?

After some more thoughts, I think adding a new function to allocator interface 
for `updateQuota` seems cleaner than reusing this function, especially if more 
quota related enhance in the future could make the two case deviate further.

Do you think this is worth a new interface function on allcator interface?


> On Oct. 20, 2016, 3:53 p.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, lines 1028-1032
> > 
> >
> > We definitely need an override taking a master call, hence we should 
> > update the operator API as well. Feel free to do it either here or as a 
> > separate commit. You can look into https://reviews.apache.org/r/49247 for 
> > inspiration.

Will do in a follow up diff.


> On Oct. 20, 2016, 3:53 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 560-563
> > 
> >
> > This function is almost identical to `_set()`. Have you explored the 
> > possibility to calculate the delta (new - old) and re-use the set path if 
> > the delta > 0 and remove path otherwise?
> > 
> > If we can't express update internally in terms of set and remove, we 
> > should better maintain a separate update path (including allocator) for 
> > simplicity.

I perfer a separate update path. I think reusing `set` and `remove` path is 
somehow breaking the current allocator interface without too much benefit, and 
I'm not sure we will end up with more readable code.


- Zhitao


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


On Oct. 19, 2016, 11:28 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52103/
> ---
> 
> (Updated Oct. 19, 2016, 11:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
> 
> 
> Bugs: MESOS-4941
> https://issues.apache.org/jira/browse/MESOS-4941
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement quota update through `PUT` method.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
>   src/master/http.cpp 9005e7c308d5f57c6f5c573951d468a3ba730740 
>   src/master/master.hpp 35db198748b8652eb53e17f592f6b40d1e6a3ed9 
>   src/master/quota_handler.cpp bf6a613a7bb3c62fd77d1ffde3170749d6c21fa2 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
> 
> Diff: https://reviews.apache.org/r/52103/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53691: Implemented some quota functionality tests.

[Zhitao Li]

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




src/tests/master_quota_tests.cpp (line 1712)


@alexr, can you please advise what is the best way for the "alert and under 
quota situation" check in your mind here?

Given that there is only one agent, there is little we could do in such a 
case.


- Zhitao Li


On Nov. 12, 2016, 7:59 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53691/
> ---
> 
> (Updated Nov. 12, 2016, 7:59 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
> 
> 
> Bugs: MESOS-4292
> https://issues.apache.org/jira/browse/MESOS-4292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This implements some tests in previous todos for
> quota functionality.
> 
> The one for:
> 
> `Role quota is below its allocation (InverseOffer generation).`
> 
> does not seem be implementable right now, since
> hierarchical allocator does not send InverseOffer for
> quota yet.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
> 
> Diff: https://reviews.apache.org/r/53691/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>