Re: Review Request 50599: Added a new 'device' entry to 'docker::Flags'.

[Yubo Li]

> On 八月 1, 2016, 10:12 a.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, line 229
> > 
> >
> > You are losing the logic of handling the `device` here: 
> > 
> > ```
> > // Exposed devices to this docker container.
> > dockerFlags.device = device;
> > ```
> > 
> > I'd prefer that you add the `device` flags first before implementing 
> > `dockerFlags`.

I Re-ordered commits so that this logic is added to this commit, after 
`--device` implemented by `mesos-docker-executor`.


> On 八月 1, 2016, 10:12 a.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 1362-1366
> > 
> >
> > ```
> > const string nvidiaData = nvidiaDataPrefix +
> >   boost::lexical_cast(gpu.minor);
> > 
> > Docker::Device nvidiaDataDevice = Docker::Device(
> > nvidiaData, nvidiaData, permission);
> > 
> > argv.push_back(nvidiaDataDevice.serialize());
> > ```

Fixed, also change `Docker::Device nvidiaDataDevice = 
Docker::Device(nvidiaData, nvidiaData, permission);` to `const`.


> On 八月 1, 2016, 10:12 a.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, line 1371
> > 
> >
> > I'd prefer that you show an example here as the comment for the 
> > `nvidiaGpuDevices`.

Added following comments:
1378 // "nvidiaGpuDevices" indicates Nvidia GPU devices need
1379 // to be exposed in the docker container. For example, to
1380 // expose Nvidia GPU0, "nvidiaGpuDevices" should be a string
1381 // like "/dev/nvidiactl:/dev/nvidiactl:mrw,/dev/nvidia-uvm:
1382 // /dev/nvidia-uvm:mrw,/dev/nvidia0:/dev/nvidia0:mrw"


- Yubo


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


On 七月 29, 2016, 9:54 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50599/
> ---
> 
> (Updated 七月 29, 2016, 9:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new string entry 'device' to 'docker::Flags' to
> indicate host devices exposed into docker container, and
> passed allocated GPUs to it.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 
> 
> Diff: https://reviews.apache.org/r/50599/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50599: Added a new 'device' entry to 'docker::Flags'.

[Guangya Liu]

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




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


You are losing the logic of handling the `device` here: 

```
// Exposed devices to this docker container.
dockerFlags.device = device;
```

I'd prefer that you add the `device` flags first before implementing 
`dockerFlags`.



src/slave/containerizer/docker.cpp (lines 1353 - 1354)


```
const Docker::Device nvidiaCtlDevice = Docker::Device(
nvidiaCtl, nvidiaCtl, permission);
```



src/slave/containerizer/docker.cpp (lines 1354 - 1355)


new line here



src/slave/containerizer/docker.cpp (lines 1357 - 1358)


```
const Docker::Device nvidiaUvmDevice = Docker::Device(
nvidiaUvm, nvidiaUvm, permission);
```



src/slave/containerizer/docker.cpp (lines 1358 - 1359)


new line here



src/slave/containerizer/docker.cpp (lines 1362 - 1366)


```
const string nvidiaData = nvidiaDataPrefix +
  boost::lexical_cast(gpu.minor);

Docker::Device nvidiaDataDevice = Docker::Device(
nvidiaData, nvidiaData, permission);

argv.push_back(nvidiaDataDevice.serialize());
```



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


I'd prefer that you show an example here as the comment for the 
`nvidiaGpuDevices`.


- Guangya Liu


On 七月 29, 2016, 9:54 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50599/
> ---
> 
> (Updated 七月 29, 2016, 9:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new string entry 'device' to 'docker::Flags' to
> indicate host devices exposed into docker container, and
> passed allocated GPUs to it.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 
> 
> Diff: https://reviews.apache.org/r/50599/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50599: Added a new 'device' entry to 'docker::Flags'.

[Yubo Li]

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

(Updated 七月 29, 2016, 9:54 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


Changes
---

Splitted a new patch for atomic function


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


Repository: mesos


Description
---

Added a new string entry 'device' to 'docker::Flags' to
indicate host devices exposed into docker container, and
passed allocated GPUs to it.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 

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


Testing
---

make check


Thanks,

Yubo Li

Review Request 50599: Added a new 'device' entry to 'docker::Flags'.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


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


Repository: mesos


Description
---

Added a new string entry 'device' to 'docker::Flags' to
indicate host devices exposed into docker container, and
passed allocated GPUs to it.


Diffs
-

  src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 

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


Testing
---

make check


Thanks,

Yubo Li