You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/06/02 01:11:01 UTC
Review Request 34910: Added task validation for task using revocable
resources while its executor does not.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
Bugs: MESOS-2753
https://issues.apache.org/jira/browse/MESOS-2753
Repository: mesos
Description
-------
Enforces the invariant that a task cannot use revocable resources unless its executor does.
Diffs
-----
src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
Diff: https://reviews.apache.org/r/34910/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Ben Mahler <be...@gmail.com>.
> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 1136-1137
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1136>
> >
> > I find it nice to reduce "jaggedness" on comments, e.g.:
> >
> > ```
> > // Create a task that uses revocable resources
> > // while it's executor does not.
> > ```
>
> Vinod Kone wrote:
> I think most of our code just wraps the comments at 70.
That is true, as that is the only hard rule we have (although it's not getting enforced everywhere :)).
But subjectively, let's try to write readable comments that are less "jagged", in the same spirit as we do with our source code wrapping! Maybe this means I'm signing up for adding to the style guide ;)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/#review86449
-----------------------------------------------------------
On June 10, 2015, 11:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34910/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 11:56 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-2753
> https://issues.apache.org/jira/browse/MESOS-2753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enforces the invariant that a task cannot use revocable resources unless its executor does.
>
>
> Diffs
> -----
>
> src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765
> src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
>
> Diff: https://reviews.apache.org/r/34910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > Can we make the test a unit test? Looks like we could pull up '`validateResources`' to make this unit-testable? Chatting with Jie, we should probably place it in an `'internal::task'` namespace towards the bottom of the header, since it's only for testing purposes and we want to make it clear which validation functions are expected to be used by the master.
done.
> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/master/validation.cpp, lines 321-326
> > <https://reviews.apache.org/r/34910/diff/1/?file=975981#file975981line321>
> >
> > Are we otherwise allowing mixing within a resource? Looking at the existing cpushare.cpp logic, it seems that we have a binary decision:
> >
> > (1) If any of the cpus are revocable, *all* cpus are treated revocable for isolation purposes.
> > (2) Otherwise, non-revocable.
> >
> > Is the idea here that the mixing that (1) allows enables a framework to reduce the "risk" of revocation?
> >
> > Maybe a TODO here and/or on the cpushare code since it's not that difficult to change the cpushare implementation to allow transitioning between revocable and non-revocable after the container is launched?
per the latest discussion, we are not allowing mixing of resources (revocable and non-revocable) for any given type (e.g., cpus).
> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, line 1105
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1105>
> >
> > its :)
N/A.
> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 1136-1137
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1136>
> >
> > I find it nice to reduce "jaggedness" on comments, e.g.:
> >
> > ```
> > // Create a task that uses revocable resources
> > // while it's executor does not.
> > ```
I think most of our code just wraps the comments at 70.
> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 1104-1105
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1104>
> >
> > From this comment, I expected this test to be testing that a task with revocable resources is allowed when the executor has revocable resources, but it's only testing the opposite condition.
> >
> > The unit test would make it easier to test both cases, yes?
added tests for both cases.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/#review86449
-----------------------------------------------------------
On June 1, 2015, 11:11 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34910/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 11:11 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-2753
> https://issues.apache.org/jira/browse/MESOS-2753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enforces the invariant that a task cannot use revocable resources unless its executor does.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
> src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
>
> Diff: https://reviews.apache.org/r/34910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/#review86449
-----------------------------------------------------------
Can we make the test a unit test? Looks like we could pull up '`validateResources`' to make this unit-testable? Chatting with Jie, we should probably place it in an `'internal::task'` namespace towards the bottom of the header, since it's only for testing purposes and we want to make it clear which validation functions are expected to be used by the master.
src/master/validation.cpp
<https://reviews.apache.org/r/34910/#comment138437>
Are we otherwise allowing mixing within a resource? Looking at the existing cpushare.cpp logic, it seems that we have a binary decision:
(1) If any of the cpus are revocable, *all* cpus are treated revocable for isolation purposes.
(2) Otherwise, non-revocable.
Is the idea here that the mixing that (1) allows enables a framework to reduce the "risk" of revocation?
Maybe a TODO here and/or on the cpushare code since it's not that difficult to change the cpushare implementation to allow transitioning between revocable and non-revocable after the container is launched?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/34910/#comment138453>
From this comment, I expected this test to be testing that a task with revocable resources is allowed when the executor has revocable resources, but it's only testing the opposite condition.
The unit test would make it easier to test both cases, yes?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/34910/#comment138443>
its :)
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/34910/#comment138439>
I find it nice to reduce "jaggedness" on comments, e.g.:
```
// Create a task that uses revocable resources
// while it's executor does not.
```
- Ben Mahler
On June 1, 2015, 11:11 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34910/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 11:11 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-2753
> https://issues.apache.org/jira/browse/MESOS-2753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enforces the invariant that a task cannot use revocable resources unless its executor does.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
> src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
>
> Diff: https://reviews.apache.org/r/34910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 10, 2015, 10:03 p.m., Jie Yu wrote:
> > src/master/validation.cpp, lines 714-721
> > <https://reviews.apache.org/r/34910/diff/2/?file=982185#file982185line714>
> >
> > Is this check redundent? Can you calculate the total first and do one check instead?
I originally just had one. But opted to split the check for a better/concise error message. I'll just update the error message and have just one check.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/#review87465
-----------------------------------------------------------
On June 10, 2015, 7:10 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34910/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 7:10 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-2753
> https://issues.apache.org/jira/browse/MESOS-2753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enforces the invariant that a task cannot use revocable resources unless its executor does.
>
>
> Diffs
> -----
>
> src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765
> src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
>
> Diff: https://reviews.apache.org/r/34910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/#review87465
-----------------------------------------------------------
Ship it!
src/master/validation.hpp
<https://reviews.apache.org/r/34910/#comment139777>
Can you instead use namespace task::internal? See my comments below.
src/master/validation.cpp
<https://reviews.apache.org/r/34910/#comment139781>
This looks wiered. Can we instead put all the validateXXX function under task::internal namespace? You can keep validateResources close to validateResourceUsage as it is right now.
And this code block will look more consistent:
```
lambda::bind(internal::validateTaskID, ...),
lambda::bind(internal::validateUnique...),
...
```
src/master/validation.cpp
<https://reviews.apache.org/r/34910/#comment139782>
See my above comments. You may want to move this close to where all the task validation functions are located.
src/master/validation.cpp
<https://reviews.apache.org/r/34910/#comment139786>
Is this check redundent? Can you calculate the total first and do one check instead?
src/master/validation.cpp
<https://reviews.apache.org/r/34910/#comment139790>
I would rather swap the order of these two checks:
```
if (!resources.revocable.empty() &&
resources.revocable() != resources)
```
I found the above more easy to read:)
- Jie Yu
On June 10, 2015, 7:10 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34910/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 7:10 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-2753
> https://issues.apache.org/jira/browse/MESOS-2753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enforces the invariant that a task cannot use revocable resources unless its executor does.
>
>
> Diffs
> -----
>
> src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765
> src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
>
> Diff: https://reviews.apache.org/r/34910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/
-----------------------------------------------------------
(Updated June 10, 2015, 11:56 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
Changes
-------
rebased.
Bugs: MESOS-2753
https://issues.apache.org/jira/browse/MESOS-2753
Repository: mesos
Description
-------
Enforces the invariant that a task cannot use revocable resources unless its executor does.
Diffs (updated)
-----
src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765
src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28
src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
Diff: https://reviews.apache.org/r/34910/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/
-----------------------------------------------------------
(Updated June 10, 2015, 11:18 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
Changes
-------
jie's. NNFR.
Bugs: MESOS-2753
https://issues.apache.org/jira/browse/MESOS-2753
Repository: mesos
Description
-------
Enforces the invariant that a task cannot use revocable resources unless its executor does.
Diffs (updated)
-----
src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765
src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28
src/slave/slave.cpp bb9dd07a9c5ce0d82016809d9eb647b2c64b2d8f
src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
Diff: https://reviews.apache.org/r/34910/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 34910: Added task validation for task using
revocable resources while its executor does not.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/
-----------------------------------------------------------
(Updated June 10, 2015, 7:10 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
Changes
-------
updated the invariant to not allow mixing of revocable and non-revocable resources.
Bugs: MESOS-2753
https://issues.apache.org/jira/browse/MESOS-2753
Repository: mesos
Description
-------
Enforces the invariant that a task cannot use revocable resources unless its executor does.
Diffs (updated)
-----
src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765
src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe
Diff: https://reviews.apache.org/r/34910/diff/
Testing
-------
make check
Thanks,
Vinod Kone