Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-29 Thread Ben Mahler 

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


Ship it!




Went over this with kevin, we'll be making some minor adjustments to the 
comments.


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


how about "millis" to try to make the intent of the modulo logic a bit more 
clear?



src/slave/containerizer/containerizer.cpp (lines 117 - 119)


We would probably need this TODO in all of our nvidia gpu blocks :)


- Ben Mahler


On March 14, 2016, 7:39 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 14, 2016, 7:39 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, we enforce that the number of GPUs specified in the 'gpus'
> resource parameter equal the number of GPUs passed in via the
> --nvidia_gpu_devices flag. In the future, we will generalize this via
> autodiscovery of GPUs and support for GPU types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bb343ad852576a75615a93ef850b413bf77698e0 
>   include/mesos/v1/resources.hpp 719110fbbf39f1755460ac0b32e3893656054a4e 
>   src/common/resources.cpp 1f23dc83f7330c305a836d698f114b7eaf3d7ba1 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp c6f125ec317e2da537a6456f5cff2da0a48701d8 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:string"
> Failed to determine slave resources: Bad type for resource gpus value string 
> type TEXT
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.9"
> Failed to determine slave resources: The `gpus` resource must specified as an 
> unsigned integer
>
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0"
> Failed to determine slave resources: When specifying the `gpus` resource, you 
> must also specify a list of GPUs via the `--nvidia_gpu_devices` flag
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
> --nvidia_gpu_devices=1,2,3
> Failed to determine slave resources: The number of GPUs passed in the 
> `--nvidia_gpu_devices` flag must match the number of GPUs specified in the 
> `gpus` resource
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
> --nvidia_gpu_devices=1,2,3,4
> SUCCESS
> 
> **NOTE**: I didn't set the `--isolation` flag here, so the agent actually 
> started up correctly (i.e. it didn't error out saying that the Nvidia GPU 
> isolator is currently unsupported).  This is the correct behaviour, and it 
> properly exercises the code added in this patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-14 Thread Kevin Klues 


> On March 8, 2016, 10:45 p.m., Vikrama Ditya wrote:
> > src/slave/containerizer/containerizer.cpp, line 107
> > 
> >
> > It will be good to comment that --nvidia-gpus flag consists of devices 
> > numbers as comes out from nvidia-smi output. Good to give example here.

This is documented in `docs/configuration.md`  as well as in the flags 
parameter itself (i.e. what you see if you run `mesos-slave --help`). You can 
see the review for these additions at: https://reviews.apache.org/r/44365

I'm not sure it adds much value to duplicate that information here, since the 
numbers themselves aren't actually consumed at all.  We should maybe mention it 
again though in the isolator where they are actually used.


- Kevin


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


On March 14, 2016, 7:39 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 14, 2016, 7:39 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, we enforce that the number of GPUs specified in the 'gpus'
> resource parameter equal the number of GPUs passed in via the
> --nvidia_gpu_devices flag. In the future, we will generalize this via
> autodiscovery of GPUs and support for GPU types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bb343ad852576a75615a93ef850b413bf77698e0 
>   include/mesos/v1/resources.hpp 719110fbbf39f1755460ac0b32e3893656054a4e 
>   src/common/resources.cpp 1f23dc83f7330c305a836d698f114b7eaf3d7ba1 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp c6f125ec317e2da537a6456f5cff2da0a48701d8 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:string"
> Failed to determine slave resources: Bad type for resource gpus value string 
> type TEXT
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.9"
> Failed to determine slave resources: The `gpus` resource must specified as an 
> unsigned integer
>
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0"
> Failed to determine slave resources: When specifying the `gpus` resource, you 
> must also specify a list of GPUs via the `--nvidia_gpu_devices` flag
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
> --nvidia_gpu_devices=1,2,3
> Failed to determine slave resources: The number of GPUs passed in the 
> `--nvidia_gpu_devices` flag must match the number of GPUs specified in the 
> `gpus` resource
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
> --nvidia_gpu_devices=1,2,3,4
> SUCCESS
> 
> **NOTE**: I didn't set the `--isolation` flag here, so the agent actually 
> started up correctly (i.e. it didn't error out saying that the Nvidia GPU 
> isolator is currently unsupported).  This is the correct behaviour, and it 
> properly exercises the code added in this patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-14 Thread Kevin Klues 


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > Could you update the description to reflect the new state of the code?

Done.


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, line 96
> > 
> >
> > Can we also prevent fractional GPUs here?
> > 
> > We'd also need to do fractional validation in the launch task path, 
> > inside `update` (as we discussed `prepare` is calling update so no need to 
> > stick it in both functions).

I've added code to do the check here.  We will need to remember to add it into 
the `prepare` function once we fill that function in.


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, lines 97-102
> > 
> >
> > Since it's unlikely we'll have a default number of GPUs in the future, 
> > perhaps we just have a comment here that describes why we don't use a 
> > default?

Removed and comment added.


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, line 114
> > 
> >
> > Needs to be in the ifdef, right?

Done


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, lines 114-135
> > 
> >
> > Can you remove the periods at the end of the error messages?

Done


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, lines 115-118
> > 
> >
> > Our pattern for error messages is to avoid the periods and 
> > multi-sentence messages. We'll try to write messages that can compose as 
> > follows:
> > 
> > Failed to : 
> > 
> > Where reason can also correspond to a composed message, so you can end 
> > up with things like:
> > 
> > Failed to launch container: Failed to prepare container: Fractional 
> > 'gpus' not supported.
> > 
> > So when we've needed multi-sentence messages, we've tended to use 
> > semi-colons.

These errors have been reduced to single sentences now, so this is no longer an 
issue. Good to keep in mind though.


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, line 118
> > 
> >
> > These should be plural, so 'gpus'

Fixed.


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/slave/containerizer/containerizer.cpp, lines 121-130
> > 
> >
> > We can get rid of the parsing code here based on the suggestion in the 
> > previous review to add to common/parse.hpp.
> > 
> > Note that we would usually use const& in the loop here.

Its gone now. The check below it still remains, but now it just checks the size 
of the array from the flag directly (since the flag is a `vector`)


> On March 9, 2016, 1:14 a.m., Ben Mahler wrote:
> > src/v1/resources.cpp, line 1238
> > 
> >
> > Don't feel obligated, but would be nice to do a blanket 
> > s/`.get().`/`->`/ cleanup in the resources / values files, separate from 
> > this patch.

I'll add it as a pre-patch to this patch set. That way the new GPU resource 
that's being added will never do it the "wrong" way.

Turns out there is nothing to be done in the values files, just the resources 
files. Also, doing a quick grep, it seems like we still do things the "wrong" 
way all over the place.  Might be nice to do a pass of the full source tree and 
kill all birds with one stone at some point.


- Kevin


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


On March 14, 2016, 7:39 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 14, 2016, 7:39 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, we enforce that the number of GPUs specified in the 'gpus'
> resource parameter equal the number of GPUs passed in via the
> --nvidia_gpu_devices flag. In the future, we will generalize this via
> autodiscovery of GPUs and support for

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-14 Thread Kevin Klues 

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

(Updated March 14, 2016, 7:39 a.m.)


Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.


Changes
---

Adressed all of klaus1982, bmahler, and vditya's comments.


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


Repository: mesos


Description (updated)
---

Currently, we enforce that the number of GPUs specified in the 'gpus'
resource parameter equal the number of GPUs passed in via the
--nvidia_gpu_devices flag. In the future, we will generalize this via
autodiscovery of GPUs and support for GPU types other than Nvidia.


Diffs (updated)
-

  include/mesos/resources.hpp bb343ad852576a75615a93ef850b413bf77698e0 
  include/mesos/v1/resources.hpp 719110fbbf39f1755460ac0b32e3893656054a4e 
  src/common/resources.cpp 1f23dc83f7330c305a836d698f114b7eaf3d7ba1 
  src/slave/containerizer/containerizer.cpp 
f6fc7863d0c215611f170dc0c89aa229407b5137 
  src/v1/resources.cpp c6f125ec317e2da537a6456f5cff2da0a48701d8 

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


Testing (updated)
---

./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:string"
Failed to determine slave resources: Bad type for resource gpus value string 
type TEXT

./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.9"
Failed to determine slave resources: The `gpus` resource must specified as an 
unsigned integer
   
./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0"
Failed to determine slave resources: When specifying the `gpus` resource, you 
must also specify a list of GPUs via the `--nvidia_gpu_devices` flag

./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
--nvidia_gpu_devices=1,2,3
Failed to determine slave resources: The number of GPUs passed in the 
`--nvidia_gpu_devices` flag must match the number of GPUs specified in the 
`gpus` resource

./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
--nvidia_gpu_devices=1,2,3,4
SUCCESS

**NOTE**: I didn't set the `--isolation` flag here, so the agent actually 
started up correctly (i.e. it didn't error out saying that the Nvidia GPU 
isolator is currently unsupported).  This is the correct behaviour, and it 
properly exercises the code added in this patch.


Thanks,

Kevin Klues

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-08 Thread Ben Mahler 

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



Could you update the description to reflect the new state of the code?


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


Can we also prevent fractional GPUs here?

We'd also need to do fractional validation in the launch task path, inside 
`update` (as we discussed `prepare` is calling update so no need to stick it in 
both functions).



src/slave/containerizer/containerizer.cpp (lines 97 - 102)


Since it's unlikely we'll have a default number of GPUs in the future, 
perhaps we just have a comment here that describes why we don't use a default?



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


Needs to be in the ifdef, right?



src/slave/containerizer/containerizer.cpp (lines 114 - 135)


Can you remove the periods at the end of the error messages?



src/slave/containerizer/containerizer.cpp (lines 115 - 118)


Our pattern for error messages is to avoid the periods and multi-sentence 
messages. We'll try to write messages that can compose as follows:

Failed to : 

Where reason can also correspond to a composed message, so you can end up 
with things like:

Failed to launch container: Failed to prepare container: Fractional 'gpus' 
not supported.

So when we've needed multi-sentence messages, we've tended to use 
semi-colons.



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


These should be plural, so 'gpus'



src/slave/containerizer/containerizer.cpp (lines 121 - 130)


We can get rid of the parsing code here based on the suggestion in the 
previous review to add to common/parse.hpp.

Note that we would usually use const& in the loop here.



src/v1/resources.cpp (line 1238)


Don't feel obligated, but would be nice to do a blanket s/`.get().`/`->`/ 
cleanup in the resources / values files, separate from this patch.


- Ben Mahler


On March 8, 2016, 10:47 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 8, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-08 Thread Vikrama Ditya 

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




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


It will be good to comment that --nvidia-gpus flag consists of devices 
numbers as comes out from nvidia-smi output. Good to give example here.


- Vikrama Ditya


On March 4, 2016, 1:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-05 Thread Kevin Klues 


> On March 4, 2016, 12:27 p.m., Klaus Ma wrote:
> >

OK, got it.  I'll wait for more comments and then do a full pass all at once.  
Thanks!


- Kevin


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


On March 4, 2016, 1:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-05 Thread Klaus Ma 


> On March 4, 2016, 8:27 p.m., Klaus Ma wrote:
> > src/slave/containerizer/containerizer.cpp, line 115
> > 
> >
> > I'd like to say "`--navidia_gpus` must be set when specifying `gpu` 
> > resources."
> 
> Kevin Klues wrote:
> Are you just suggesting to make the Error string shorter?

Yes :).


> On March 4, 2016, 8:27 p.m., Klaus Ma wrote:
> > src/slave/containerizer/containerizer.cpp, line 105
> > 
> >
> > `` for --nvidia_gpus; not only this line, please also update them in 
> > the following comments.
> 
> Kevin Klues wrote:
> Sorry, I don't think I understand.  What are you proposing here?

According to the comments style, we need to add `` for --nvidia_gpus; 
s/--nvidia_gpus/`--nvidia_gpus`/g


- Klaus


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


On March 4, 2016, 9:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 9:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-04 Thread Kevin Klues 


> On March 4, 2016, 12:27 p.m., Klaus Ma wrote:
> > src/slave/containerizer/containerizer.cpp, line 105
> > 
> >
> > `` for --nvidia_gpus; not only this line, please also update them in 
> > the following comments.

Sorry, I don't think I understand.  What are you proposing here?


> On March 4, 2016, 12:27 p.m., Klaus Ma wrote:
> > src/slave/containerizer/containerizer.cpp, line 115
> > 
> >
> > I'd like to say "`--navidia_gpus` must be set when specifying `gpu` 
> > resources."

Are you just suggesting to make the Error string shorter?


- Kevin


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


On March 4, 2016, 1:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-04 Thread Klaus Ma 

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




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


`` for --nvidia_gpus; not only this line, please also update them in the 
following comments.



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


I'd like to say "`--navidia_gpus` must be set when specifying `gpu` 
resources."


- Klaus Ma


On March 4, 2016, 9:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 9:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-04 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [44360, 44361, 44363, 44364, 44365, 44366]

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 March 4, 2016, 1:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 44366: Added GPUs as an explicit resource.

2016-03-03 Thread Kevin Klues 

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

Review request for mesos, Ben Mahler and Rob Todd.


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


Repository: mesos


Description
---

Currently we can either infer the number of GPUs to offer as resources
from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
resource. In the future, we will generalize this via autodiscovery of
gpus and support for gpus types other than Nvidia.


Diffs
-

  include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
  include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
  src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
  src/slave/containerizer/containerizer.cpp 
f6fc7863d0c215611f170dc0c89aa229407b5137 
  src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 

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


Testing
---


Thanks,

Kevin Klues