You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2017/01/23 22:47:03 UTC
Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
Bugs: MESOS-6967
https://issues.apache.org/jira/browse/MESOS-6967
Repository: mesos
Description
-------
Previously, `Resource` did not contain `AllocationInfo`. So for
backwards compatibility with old schedulers and tooling, we must
allow operations to contain `Resource`s without an allocation role.
The two interesting cases for adjusting the operation's resource are:
(1) The operation `Resource` does not contain an `AllocationInfo`
but is being applied to an allocated `Resources`. We allow this
only if the operation is unambiguous, that is, the allocated
`Resources` are only allocated to a single role.
(2) The operation `Resource` contains an `AllocationInfo` but is
being applied to an unallocated `Resources`. In this case we
simply ignore the `AllocationInfo` of the `Resource`.
Note that we assume no `Resources` store a mix of allocated and
unallocated resources. This is a brittle assumption that we should
have enforcement for.
Diffs
-----
src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
Diff: https://reviews.apache.org/r/55828/diff/
Testing
-------
Added a test.
Thanks,
Benjamin Mahler
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 28, 2017, 3:35 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 1274-1276
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1274>
> >
> > This indicates that if the `Resource` do not have `AllocationInfo`, then we will loop all of the `Resource` till finished then return false, but we have the assumption of no `Resources` store a mix of allocated and unallocated resources, so how about `return resource.has_allocation_info();` here?
I was a bit hestitant to say it's not allocated if one but not all of the resources have allocation info. I agree though that we need to enforce the invariant or have separate types for allocated and unallocated resources to prevent mixing, at which point we can write the more efficient version.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163236
-----------------------------------------------------------
On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163236
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/common/resources.cpp (lines 1274 - 1276)
<https://reviews.apache.org/r/55828/#comment234819>
This indicates that if the `Resource` do not have `AllocationInfo`, then we will loop all of the `Resource` till finished then return false, but we have the assumption of no `Resources` store a mix of allocated and unallocated resources, so how about `return resource.has_allocation_info();` here?
src/tests/resources_utils.hpp (line 27)
<https://reviews.apache.org/r/55828/#comment234690>
I think that we do not like `using xxx` in a header file.
src/tests/resources_utils.hpp (line 33)
<https://reviews.apache.org/r/55828/#comment234691>
I think that we can kill the keyword `TODO` here?
- Guangya Liu
On \u4e00\u6708 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 23, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163898
-----------------------------------------------------------
Filed https://issues.apache.org/jira/browse/MESOS-7048 to move the stripping of allocation info up into the call-sites rather than putting it inside `Resources::apply()`.
- Benjamin Mahler
On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, lines 1263-1266
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1263>
> >
> > Could you clarify under what conditions this can happen?
> >
> > It seems surprising to silently drop information explicitly given by users, and I believe rejecting the operation would be clearer. If situations can occur where a user has more recent information than the current master (e.g., an allocation info not yet known to the master) we should fix failover behavior.
There are two intake paths for operations:
(1) When a framework accepts an offer, and
(2) When an operator uses an endpoint to apply an operation (e.g. reserve, create volume, etc).
In case (1) we have to apply the operation to the allocated resources. This will fail if the allocation info is incorrect, and if it's absent we will inject it for them prior to applying it (this at least needs to be done for old-style frameworks, but it is done for MULTI_ROLE frameworks as well since it seemed like an easier upgrade path for schedulers since they don't need to ensure they carry or set the allocation info into the operations when they accept). However, we then need to update our internal state for the agent's total and allocated resources. The former has no allocation info and the latter has a variety of allocation infos. Because we have to apply the same operation to both, we either need to handle this within `Resources::apply()` or each call site needs to strip the allocation info prior to applying to the total. I went with the former approach because it seemed easier on the callers, but given the confusion here, perhaps it is clearer to impose the stripping on the
callers. I'll add a TODO. Make sense?
In case (2) this doesn't occur.
> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1288
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1288>
> >
> > This looks like an expensive computation. Should we precompute it at function scope and capture it here by ref?
Agreed, the issue here is that the current implementation of allocations() CHECK fails if the resources are unallocated, so calling it blindly will crash.
For now I'll put a TODO.
> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/tests/resources_utils.hpp, line 40
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613179#file1613179line40>
> >
> > This class seems confusing. It e.g., is not a wrapper since when constructing an `AllocatedResources` from some unallocated `Resources` an assertion like `EXPECT_EQ(resources, allocated)` is not true (i.e., it doesn't just wrap some `Resources` and some role).
> >
> > What about making this a function for now, e.g.,
> >
> > Resources allocatedResources(
> > const Resources& resources,
> > const string& role);
I'm not sure I followed the point about the EXPECT_EQ, but the intention was to signal to the reader that we may want a type for our flavors of resoures (allocated vs unallocated) in order to prevent invalid operations and the mixing of unallocated and allocated resources. I'll make it a function for now since we don't have a use for the class.
> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1272
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1272>
> >
> > Did you intend to make this `const`? If not, the following appears more straight-forward to me,
> >
> > bool isAllocated = false;
> > foreach(const Resource& resource, resources) {
> > if (resource.has_allocation_info()) {
> > isAllocated = true;
> > break;
> > }
> > }
> >
> > If you'd went for the optimization suggested by Guangya this does become a "one-liner" though,
> >
> > const bool isAllocated =
> > resources.resources().empty()
> > ? false
> > : resources.resources(0).has_allocation_info();
Yeah, all of these sound fine to me, however I was looking to signal to the reader that isAllocated is a function applied to Resources. If we had better enforcement of the invariant that there is no mixing of resources then the optimization would sound good to me.
I'll make it const.
> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, lines 1268-1270
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1268>
> >
> > `..., see MESOS-6636.` to allow better tracking of the status of this.
Hm.. MESOS-6636 is about enforcing that executors and tasks do not mix resources within themselves, which will be enforced at the validation layer. What I'm saying here is that our `Resources` class itself doesn't enforce the invariant of no mixing but our code generally does not do any mixing and likely should not need any mixing because of it being error prone.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163503
-----------------------------------------------------------
On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1288
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1288>
> >
> > This looks like an expensive computation. Should we precompute it at function scope and capture it here by ref?
>
> Benjamin Mahler wrote:
> Agreed, the issue here is that the current implementation of allocations() CHECK fails if the resources are unallocated, so calling it blindly will crash.
> For now I'll put a TODO.
Actually, this is no longer needed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163503
-----------------------------------------------------------
On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163503
-----------------------------------------------------------
src/common/resources.cpp (lines 1263 - 1266)
<https://reviews.apache.org/r/55828/#comment234974>
Could you clarify under what conditions this can happen?
It seems surprising to silently drop information explicitly given by users, and I believe rejecting the operation would be clearer. If situations can occur where a user has more recent information than the current master (e.g., an allocation info not yet known to the master) we should fix failover behavior.
src/common/resources.cpp (lines 1268 - 1270)
<https://reviews.apache.org/r/55828/#comment234973>
`..., see MESOS-6636.` to allow better tracking of the status of this.
src/common/resources.cpp (line 1272)
<https://reviews.apache.org/r/55828/#comment234971>
Did you intend to make this `const`? If not, the following appears more straight-forward to me,
bool isAllocated = false;
foreach(const Resource& resource, resources) {
if (resource.has_allocation_info()) {
isAllocated = true;
break;
}
}
If you'd went for the optimization suggested by Guangya this does become a "one-liner" though,
const bool isAllocated =
resources.resources().empty()
? false
: resources.resources(0).has_allocation_info();
src/common/resources.cpp (line 1288)
<https://reviews.apache.org/r/55828/#comment234972>
This looks like an expensive computation. Should we precompute it at function scope and capture it here by ref?
src/tests/resources_utils.hpp (line 40)
<https://reviews.apache.org/r/55828/#comment234969>
This class seems confusing. It e.g., is not a wrapper since when constructing an `AllocatedResources` from some unallocated `Resources` an assertion like `EXPECT_EQ(resources, allocated)` is not true (i.e., it doesn't just wrap some `Resources` and some role).
What about making this a function for now, e.g.,
Resources allocatedResources(
const Resources& resources,
const string& role);
- Benjamin Bannier
On Jan. 23, 2017, 11:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 11:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 29, 2017, 8:02 a.m., Michael Park wrote:
> > src/common/resources.cpp, lines 1257-1261
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1257>
> >
> > We talked about how this condition probably isn't necessary since we set the `AllocationInfo` in
> > `Offer::Operation`s in the master (and redundantly in the allocator).
> >
> > Please let me know how that turned out!
I can remove this one since we do the injection at the call-sites. For now I'll leave in the second case but put a TODO to consider call-sites performing the stripping of allocation info when necessary.
> On Jan. 29, 2017, 8:02 a.m., Michael Park wrote:
> > src/common/resources.cpp, line 1339
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1339>
> >
> > Shouldn't we print `unreserved` as opposed to `adjustedReservation` here?
If we were to print the pre-adjusted resource the error seems weird because it will never contain the resource. I think we'll move towards removing adjustment entirely from `Resources::apply()` and having call-site adjust (either inject or strip). I put a TODO for this.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163320
-----------------------------------------------------------
On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 29, 2017, 12:02 a.m., Michael Park wrote:
> > It took me a while trying to figure out what the invariants of `Resources` and `Offer::Operation` are, but I think I have a grasp of it.
> >
> > - We have `recovered.apply(operation);` where we have `recovered` and `operation` both with unallocated resources. (after `recovered` adjustment)
> > - We have a few `offeredResources.apply(operation);` where we have `offeredResources` and `operation` both with resources allocated to a single role. (after `operation` adjustment)
> > - We also have `totalResources.apply(operation);` where we have `totalResources` with unallocated resources, and `operation` with resources allocated to a single role. (case 2 helps us with this)
> >
> > It seems to me like we really end up with resources allocated to __multiple__ roles if we were to allow accepting multiple offers from multiple roles.
> > That is, currently the resources in `Resources` and `Offer::Operation` are either all unallocated, or all allocated to one role.
> >
> > Is this accurate?
Ah, I was wrong about `Resources`. It can hold resources that are allocated to multiple roles.
I was correct about `Offer::Operation`, however. In that they can only be unallocated or allocated to one role.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163320
-----------------------------------------------------------
On Jan. 23, 2017, 2:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 2:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55828: Updated Resources::apply to handle
Resource.AllocationInfo.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55828/#review163320
-----------------------------------------------------------
Fix it, then Ship it!
It took me a while trying to figure out what the invariants of `Resources` and `Offer::Operation` are, but I think I have a grasp of it.
- We have `recovered.apply(operation);` where we have `recovered` and `operation` both with unallocated resources. (after `recovered` adjustment)
- We have a few `offeredResources.apply(operation);` where we have `offeredResources` and `operation` both with resources allocated to a single role. (after `operation` adjustment)
- We also have `totalResources.apply(operation);` where we have `totalResources` with unallocated resources, and `operation` with resources allocated to a single role. (case 2 helps us with this)
It seems to me like we really end up with resources allocated to __multiple__ roles if we were to allow accepting multiple offers from multiple roles.
That is, currently the resources in `Resources` and `Offer::Operation` are either all unallocated, or all allocated to one role.
Is this accurate?
src/common/resources.cpp (lines 1257 - 1261)
<https://reviews.apache.org/r/55828/#comment234884>
We talked about how this condition probably isn't necessary since we set the `AllocationInfo` in
`Offer::Operation`s in the master (and redundantly in the allocator).
Please let me know how that turned out!
src/common/resources.cpp (line 1339)
<https://reviews.apache.org/r/55828/#comment234829>
Shouldn't we print `unreserved` as opposed to `adjustedReservation` here?
- Michael Park
On Jan. 23, 2017, 2:47 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 2:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
>
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
>
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
>
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
>
>
> Diffs
> -----
>
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/55828/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>