You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/06/30 09:01:35 UTC
Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7735
https://issues.apache.org/jira/browse/MESOS-7735
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
Diff: https://reviews.apache.org/r/60563/diff/1/
Testing
-------
Thanks,
Michael Park
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On June 30, 2017, 9:44 p.m., Benjamin Bannier wrote:
> > src/common/resources_utils.cpp
> > Line 199 (original), 199 (patched)
> > <https://reviews.apache.org/r/60563/diff/1/?file=1767519#file1767519line199>
> >
> > It seems by unconditionally calling `mutable_xyz` for `repeated` fields `xyz` below we might add default entries to them if they were empty before. AFAICTt this might not be what callers of this function expect.
>
> Michael Park wrote:
> Hm... do you mean the `mutable_resources()` calls below? I don't understand how we would be adding default entries. Could you be more specific?
>
> Benjamin Bannier wrote:
> Sorry, that comment was both confused/ing and put onto a too wide scope. The issue is specifically in the handling of the `LAUNCH` and `LAUNCH_GROUP` cases. There we first perform validation of resources attached to some `optional` field; this seems to handle absence of the `optional` field fine (e.g., validation of a default (empty) `repeated` list of resources is not an error), and we also do not set the `optional` field. We then iterate to mutate the resources under the `optional` field; there we would always default-init the `optional` field as a possibly unintended side-effect. AFAICT the behavior here is independent from whether one uses proto2 or proto3, but that's just from experimentation.
>
> I think we should guard the mutating resource access with a check on whether their `optional` parent message is set.
>
> Example:
>
> mesos::Operation operation;
>
> // Per default the field is not set.
> assert(!operation.has_launch());
>
> // Accessing the unset `optional` field returns a default
> // instance, but does not mutate the containing message.
> operation.launch();
> assert(!operation.has_launch());
> assert(operation.launch().task_infos_size() == 0);
> assert(!operation.has_launch());
>
> // The used mutating access changes the containing message.
> operation.mutable_launch()->mutable_task_infos();
> assert(operation.has_launch());
> assert(operation.launch().task_infos_size() == 0);
Above example is assuming e.g., the following proto definitions:
syntax = "proto2";
package mesos;
message Launch { repeated int32 task_infos = 1; }
message Operation { optional Launch launch = 1; }
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179397
-----------------------------------------------------------
On July 1, 2017, 2:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 2:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On June 30, 2017, 9:44 p.m., Benjamin Bannier wrote:
> > src/common/resources_utils.cpp
> > Line 199 (original), 199 (patched)
> > <https://reviews.apache.org/r/60563/diff/1/?file=1767519#file1767519line199>
> >
> > It seems by unconditionally calling `mutable_xyz` for `repeated` fields `xyz` below we might add default entries to them if they were empty before. AFAICTt this might not be what callers of this function expect.
>
> Michael Park wrote:
> Hm... do you mean the `mutable_resources()` calls below? I don't understand how we would be adding default entries. Could you be more specific?
Sorry, that comment was both confused/ing and put onto a too wide scope. The issue is specifically in the handling of the `LAUNCH` and `LAUNCH_GROUP` cases. There we first perform validation of resources attached to some `optional` field; this seems to handle absence of the `optional` field fine (e.g., validation of a default (empty) `repeated` list of resources is not an error), and we also do not set the `optional` field. We then iterate to mutate the resources under the `optional` field; there we would always default-init the `optional` field as a possibly unintended side-effect. AFAICT the behavior here is independent from whether one uses proto2 or proto3, but that's just from experimentation.
I think we should guard the mutating resource access with a check on whether their `optional` parent message is set.
Example:
mesos::Operation operation;
// Per default the field is not set.
assert(!operation.has_launch());
// Accessing the unset `optional` field returns a default
// instance, but does not mutate the containing message.
operation.launch();
assert(!operation.has_launch());
assert(operation.launch().task_infos_size() == 0);
assert(!operation.has_launch());
// The used mutating access changes the containing message.
operation.mutable_launch()->mutable_task_infos();
assert(operation.has_launch());
assert(operation.launch().task_infos_size() == 0);
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179397
-----------------------------------------------------------
On July 1, 2017, 2:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 2:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Michael Park <mp...@apache.org>.
> On June 30, 2017, 12:44 p.m., Benjamin Bannier wrote:
> > src/common/resources_utils.cpp
> > Line 199 (original), 199 (patched)
> > <https://reviews.apache.org/r/60563/diff/1/?file=1767519#file1767519line199>
> >
> > It seems by unconditionally calling `mutable_xyz` for `repeated` fields `xyz` below we might add default entries to them if they were empty before. AFAICTt this might not be what callers of this function expect.
Hm... do you mean the `mutable_resources()` calls below? I don't understand how we would be adding default entries. Could you be more specific?
> On June 30, 2017, 12:44 p.m., Benjamin Bannier wrote:
> > src/common/resources_utils.cpp
> > Line 201 (original), 201 (patched)
> > <https://reviews.apache.org/r/60563/diff/1/?file=1767519#file1767519line201>
> >
> > Should we `CHECK_NOTNULL` somewhere here?
Sure.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179397
-----------------------------------------------------------
On June 30, 2017, 2:01 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 2:01 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179397
-----------------------------------------------------------
src/common/resources_utils.cpp
Line 199 (original), 199 (patched)
<https://reviews.apache.org/r/60563/#comment254076>
It seems by unconditionally calling `mutable_xyz` for `repeated` fields `xyz` below we might add default entries to them if they were empty before. AFAICTt this might not be what callers of this function expect.
src/common/resources_utils.cpp
Line 201 (original), 201 (patched)
<https://reviews.apache.org/r/60563/#comment254070>
Should we `CHECK_NOTNULL` somewhere here?
- Benjamin Bannier
On June 30, 2017, 11:01 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 11:01 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179460
-----------------------------------------------------------
src/common/resources_utils.cpp
Lines 205 (patched)
<https://reviews.apache.org/r/60563/#comment254168>
Do you need all these comments?
- Benjamin Mahler
On July 1, 2017, 12:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 12:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 1, 2017, 2:25 a.m., Vinod Kone wrote:
> > src/common/resources_utils.cpp
> > Line 199 (original), 199 (patched)
> > <https://reviews.apache.org/r/60563/diff/2/?file=1767989#file1767989line199>
> >
> > I feel like validate and upgrade could be split into 2 independent helpers, but if there is no use case yet, that's fine.
Would be good to have a comment about why they're merged, wasn't obvious to me either.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179451
-----------------------------------------------------------
On July 1, 2017, 12:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 12:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179451
-----------------------------------------------------------
Ship it!
src/common/resources_utils.cpp
Line 199 (original), 199 (patched)
<https://reviews.apache.org/r/60563/#comment254147>
I feel like validate and upgrade could be split into 2 independent helpers, but if there is no use case yet, that's fine.
- Vinod Kone
On July 1, 2017, 12:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 12:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179466
-----------------------------------------------------------
src/common/resources_utils.hpp
Line 140 (original), 140 (patched)
<https://reviews.apache.org/r/60563/#comment254176>
Do you want to name this to normalize? upgrade seems to suggest we're coming from an old format, and we might not be, also the endpoint format isn't really a lower version that we're upgrading from?
- Benjamin Mahler
On July 1, 2017, 12:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 12:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/#review179458
-----------------------------------------------------------
Fix it, then Ship it!
src/common/resources_utils.hpp
Lines 135-139 (original), 135-139 (patched)
<https://reviews.apache.org/r/60563/#comment254158>
Can you add a comment about the reasoning on why we allow a (pre or post) framework to use any format? Seems pretty suprising that we would allow pre frameworks using post format, pre frameworks using endpoint format, post frameworks using pre format, etc.
Also, some cases seem to need to be prevented at some layer. For example, if a framework doesn't have the reservation refinement capability, we shouldn't let them create refined reservations, because they won't be able to see them after they create them due to our capability filtering of offers. Do we prevent pre capability frameworks from making refined reservations?
Not sure if you want to address this in this review as part of this validation, or as a separate ticket? Can you have that as a blocker for 1.4?
src/common/resources_utils.cpp
Lines 215 (patched)
<https://reviews.apache.org/r/60563/#comment254160>
This will set the 'reserve' field if it was previously unset. Seems unexpected by the caller?
It seems injectAllocationInfo has this same problem (I thought I had avoided this intentionally, but I guess not or it was lost...). It's just unfortunate from an error message perspective, since rather than telling the framework that they forgot to set the 'reserve' field, we tell them that they are providing empty resources.
src/common/resources_utils.cpp
Lines 231 (patched)
<https://reviews.apache.org/r/60563/#comment254161>
Ditto here for accidentally setting the field.
src/common/resources_utils.cpp
Lines 247 (patched)
<https://reviews.apache.org/r/60563/#comment254162>
Ditto here for accidentally setting the field.
src/common/resources_utils.cpp
Lines 263 (patched)
<https://reviews.apache.org/r/60563/#comment254163>
Ditto here for accidentally setting the field.
src/common/resources_utils.cpp
Lines 288 (patched)
<https://reviews.apache.org/r/60563/#comment254164>
Ditto here for accidentally setting the field.
src/common/resources_utils.cpp
Lines 303-304 (patched)
<https://reviews.apache.org/r/60563/#comment254165>
Ditto here for accidentally setting the field.
src/common/resources_utils.cpp
Lines 340 (patched)
<https://reviews.apache.org/r/60563/#comment254166>
Ditto here for accidentally setting the field.
src/common/resources_utils.cpp
Lines 354-356 (patched)
<https://reviews.apache.org/r/60563/#comment254167>
Hm.. is it the responsibility of this function to return an error for unknown operations? Curious how that fits into the logic in the master.
- Benjamin Mahler
On July 1, 2017, 12:56 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60563/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 12:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated `validateAndUpgradeResources` to operate on `Operation`s.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
> src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
>
>
> Diff: https://reviews.apache.org/r/60563/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60563: Updated `validateAndUpgradeResources` to
operate on `Operation`s.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60563/
-----------------------------------------------------------
(Updated June 30, 2017, 5:56 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7735
https://issues.apache.org/jira/browse/MESOS-7735
Repository: mesos
Description (updated)
-------
Updated `validateAndUpgradeResources` to operate on `Operation`s.
Diffs (updated)
-----
src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65
src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b
Diff: https://reviews.apache.org/r/60563/diff/2/
Changes: https://reviews.apache.org/r/60563/diff/1-2/
Testing
-------
Thanks,
Michael Park