Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

2017-06-22 Thread Neil Conway 

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


Fix it, then Ship it!





src/tests/master_tests.cpp
Lines 7367 (patched)


We use `EXPECT_EQ` in the other tests here.


- Neil Conway


On June 22, 2017, 10:22 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 22, 2017, 10:22 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

2017-06-22 Thread Michael Park 

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

(Updated June 22, 2017, 3:22 p.m.)


Review request for mesos and Neil Conway.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Initially, it seemed like calling `convertResourceFormat` after
operation validation seemed safe since the operation validation
themselves performed `Resources::validate` within them.

However, the rest of the operation validation code relies on
the fact that the resources have been validated, and uses
functions such as `isDynamicallyReserved`. Since functions such as
`isDynamicallyReserved` now requires "post-reservation-refinement"
format, we must perform this conversion earlier.

In this patch, we use `upgradeResources` to perform resources
validation __and__ convert the resources before going into the
operation and task validation.

We really need a better plan for this going forward. MESOS-7702.


Diffs (updated)
-

  src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf 
  src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
  src/tests/resource_offers_tests.cpp c2bbf834c1d46079af492887b9dd40e57f3f2ac7 


Diff: https://reviews.apache.org/r/60283/diff/9/

Changes: https://reviews.apache.org/r/60283/diff/8-9/


Testing
---


Thanks,

Michael Park

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

2017-06-22 Thread Neil Conway 

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


Fix it, then Ship it!





src/master/master.cpp
Line 4222 (original), 4222 (patched)


remove "after validation"



src/master/master.cpp
Lines 4760 (patched)


Per discussion, would be good to consider whether to validate/upgrade a 
task and its resources in a single operation.



src/tests/master_tests.cpp
Lines 7130 (patched)


`using` for `google::protobuf::RepeatedPtrField` ?



src/tests/master_tests.cpp
Lines 7132 (patched)


Can we add a comment here?

```
// If reservation refinement is enabled, inbound
// resources should already be in the "post-refinement" format and should 
not need to be upgraded.
```



src/tests/master_tests.cpp
Lines 7143 (patched)


Comment would be helpful here also.



src/tests/master_tests.cpp
Lines 7201 (patched)


Can we remove the `url` stuff?



src/tests/master_tests.cpp
Lines 7249 (patched)


Update or remove this comment.



src/tests/master_tests.cpp
Lines 7403 (patched)


"a RESERVE"



src/tests/master_tests.cpp
Lines 7406 (patched)


"an UNRESERVE"



src/tests/master_tests.cpp
Lines 7407 (patched)


"we test"


- Neil Conway


On June 21, 2017, 7:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

2017-06-22 Thread Neil Conway 

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




src/master/master.cpp
Line 4680 (original), 4698 (patched)


Not in this RR, but this seems dubious to me: if we're not able to launch a 
task with refined reservations on an old agent, we skip sending 
`RunTaskMessage` but we _do_ add the task to `operations`, which is later 
passed to the allocator. Seems wrong.

Also, do we have a test case for this situation?



src/master/master.cpp
Lines 4754 (patched)


I wonder if we want `validateAndUpgradeResources` of a `TaskInfo`.



src/tests/master_tests.cpp
Lines 7200 (patched)


Whitespace.



src/tests/master_tests.cpp
Lines 7202 (patched)


Can we remove this? Seems irrelevant to the test's purpose.


- Neil Conway


On June 21, 2017, 7:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

2017-06-21 Thread Neil Conway 

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




src/master/master.cpp
Line 4221 (original), 4221 (patched)


Update this comment.


- Neil Conway


On June 21, 2017, 7:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>