You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2017/03/19 04:21:58 UTC
Review Request 57752: Fixed crash with tasks that use very small
resource values.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57752/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Joris Van Remoortere.
Bugs: MESOS-7197
https://issues.apache.org/jira/browse/MESOS-7197
Repository: mesos
Description
-------
When parsing resources, inputs such as "cpus:0" are considered "empty"
and are omitted from the `Resources` object. However, a very small
resource value like "cpus:0.00001" was considered non-empty, despite the
fact that the numerical behavior for resources means that these two
values compare as equal. This inconsistency resulted in a `CHECK`
failure in the sorter.
This commit fixes the resource parsing logic to treat such very small
resource values as empty resources, which resolves the inconsistency
above. The resulting behavior might be a bit surprising to users (since
the task will launch with zero resources instead of a very small
resource value), but improving this is left as future work (MESOS-1807).
Diffs
-----
src/common/resources.cpp b64fd76d1d3d8179bae2ac5b335bdf4ad980c9a7
src/tests/master_tests.cpp 5fdb9493c9aed31b90e03062544c1446eb200040
src/tests/resources_tests.cpp 18e8b670f2f0be83be30b4a73503e786d5ae9b48
src/v1/resources.cpp 4ffe9503b0c792e832dda77e38098509d40e0f18
Diff: https://reviews.apache.org/r/57752/diff/1/
Testing
-------
`make check`; added new unit tests both for resource parsing/comparison logic and for end-to-end system behavior (launching a task with a tiny resource value and ensuring it doesn't crash the master).
I was concerned that changing `Resources::isEmpty` to introduce a local `Value::Scalar` variable would regress resource parsing performance, but that doesn't appear to be the case. I added a new microbenchmark to validate this; perf w/o the change:
```
[----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test
[ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0
[ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 (741 ms)
[ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1
[ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 (6138 ms)
[----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test (6879 ms total)
```
Perf w/ the change:
```
[----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test
[ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0
[ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 (732 ms)
[ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1
[ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 (5751 ms)
[----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test (6483 ms total)
```
(It obviously doesn't improve performance, but these results suggest to me that the change doesn't significantly regress performance, either.)
Thanks,
Neil Conway
Re: Review Request 57752: Fixed crash with tasks that use very small
resource values.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57752/#review169916
-----------------------------------------------------------
Ship it!
Ship It!
- Joris Van Remoortere
On March 19, 2017, 4:21 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57752/
> -----------------------------------------------------------
>
> (Updated March 19, 2017, 4:21 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Joris Van Remoortere.
>
>
> Bugs: MESOS-7197
> https://issues.apache.org/jira/browse/MESOS-7197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When parsing resources, inputs such as "cpus:0" are considered "empty"
> and are omitted from the `Resources` object. However, a very small
> resource value like "cpus:0.00001" was considered non-empty, despite the
> fact that the numerical behavior for resources means that these two
> values compare as equal. This inconsistency resulted in a `CHECK`
> failure in the sorter.
>
> This commit fixes the resource parsing logic to treat such very small
> resource values as empty resources, which resolves the inconsistency
> above. The resulting behavior might be a bit surprising to users (since
> the task will launch with zero resources instead of a very small
> resource value), but improving this is left as future work (MESOS-1807).
>
>
> Diffs
> -----
>
> src/common/resources.cpp b64fd76d1d3d8179bae2ac5b335bdf4ad980c9a7
> src/tests/master_tests.cpp 5fdb9493c9aed31b90e03062544c1446eb200040
> src/tests/resources_tests.cpp 18e8b670f2f0be83be30b4a73503e786d5ae9b48
> src/v1/resources.cpp 4ffe9503b0c792e832dda77e38098509d40e0f18
>
>
> Diff: https://reviews.apache.org/r/57752/diff/1/
>
>
> Testing
> -------
>
> `make check`; added new unit tests both for resource parsing/comparison logic and for end-to-end system behavior (launching a task with a tiny resource value and ensuring it doesn't crash the master).
>
> I was concerned that changing `Resources::isEmpty` to introduce a local `Value::Scalar` variable would regress resource parsing performance, but that doesn't appear to be the case. I added a new microbenchmark to validate this; perf w/o the change:
>
> ```
> [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test
> [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0
> [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 (741 ms)
> [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1
> [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 (6138 ms)
> [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test (6879 ms total)
> ```
>
> Perf w/ the change:
>
> ```
> [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test
> [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0
> [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 (732 ms)
> [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1
> [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 (5751 ms)
> [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test (6483 ms total)
> ```
>
> (It obviously doesn't improve performance, but these results suggest to me that the change doesn't significantly regress performance, either.)
>
>
> Thanks,
>
> Neil Conway
>
>