You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2019/03/08 13:34:58 UTC
Review Request 70165: Fixed operator operation handling with resource
provider resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
Bugs: MESOS-9612
https://issues.apache.org/jira/browse/MESOS-9612
Repository: mesos
Description
-------
The resource provider manager didn't allow operations originating from
operator API calls. For speculatively applied operations, this is
allowed now.
Diffs
-----
src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
Diff: https://reviews.apache.org/r/70165/diff/1/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review214043
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On March 26, 2019, 12:26 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 12:26 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87
>
>
> Diff: https://reviews.apache.org/r/70165/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review214045
-----------------------------------------------------------
PASS: Mesos patch 70165 was successfully built and tested.
Reviews applied: `['70165']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3017/mesos-review-70165
- Mesos Reviewbot Windows
On March 26, 2019, 11:26 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 11:26 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87
>
>
> Diff: https://reviews.apache.org/r/70165/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review214046
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70165]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 26, 2019, 7:26 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 7:26 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87
>
>
> Diff: https://reviews.apache.org/r/70165/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/
-----------------------------------------------------------
(Updated March 26, 2019, 12:26 p.m.)
Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
Changes
-------
Addressed issues.
Bugs: MESOS-9612
https://issues.apache.org/jira/browse/MESOS-9612
Repository: mesos
Description
-------
The resource provider manager didn't allow operations originating from
operator API calls. For speculatively applied operations, this is
allowed now.
Diffs (updated)
-----
src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
src/tests/storage_local_resource_provider_tests.cpp 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87
Diff: https://reviews.apache.org/r/70165/diff/4/
Changes: https://reviews.apache.org/r/70165/diff/3-4/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213594
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70165]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 11, 2019, 9:42 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 11, 2019, 9:42 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
>
>
> Diff: https://reviews.apache.org/r/70165/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 18, 2019, 1:09 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 5496 (patched)
> > <https://reviews.apache.org/r/70165/diff/3/?file=2130372#file2130372line5496>
> >
> > Even though above we wait for the second `UpdateSlaveMessage` we might still never get the offers we expect here.
> >
> > I was able to break this test with `stress-ng` in less than 100 iterations. /cc @chun
>
> Jan Schlicht wrote:
> This seems to affect other SLRP tests as well. We need a better way to start Mesos with a SLRP and wait for it to be ready.
Filed https://issues.apache.org/jira/browse/MESOS-9678.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213771
-----------------------------------------------------------
On March 26, 2019, 12:26 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 12:26 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87
>
>
> Diff: https://reviews.apache.org/r/70165/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213771
-----------------------------------------------------------
Thanks for the fixes, this looks good now.
I raised an issue about a possible race where the framework would never see the expected offer. Like you explained to me offline, this is an issue with other tests following the same pattern as well. @chhsia0, can you see how this could be fixed in general? Added another flaky test doesn't sound good to me.
src/resource_provider/manager.cpp
Line 512 (original), 525-526 (patched)
<https://reviews.apache.org/r/70165/#comment299758>
Nit:
```
<< " to resource provider " << resourceProviderId.get()
<< ": connection closed";
```
src/tests/storage_local_resource_provider_tests.cpp
Lines 5480 (patched)
<https://reviews.apache.org/r/70165/#comment299762>
Thanks for adding these, I think the tests very well now.
src/tests/storage_local_resource_provider_tests.cpp
Lines 5496 (patched)
<https://reviews.apache.org/r/70165/#comment299761>
Even though above we wait for the second `UpdateSlaveMessage` we might still never get the offers we expect here.
I was able to break this test with `stress-ng` in less than 100 iterations. /cc @chun
- Benjamin Bannier
On March 11, 2019, 10:42 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 11, 2019, 10:42 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
>
>
> Diff: https://reviews.apache.org/r/70165/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213701
-----------------------------------------------------------
PASS: Mesos patch 70165 was successfully built and tested.
Reviews applied: `['70165']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2943/mesos-review-70165
- Mesos Reviewbot Windows
On March 11, 2019, 9:42 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 11, 2019, 9:42 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
>
>
> Diff: https://reviews.apache.org/r/70165/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/
-----------------------------------------------------------
(Updated March 11, 2019, 10:42 a.m.)
Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
Changes
-------
Addressed issues.
Bugs: MESOS-9612
https://issues.apache.org/jira/browse/MESOS-9612
Repository: mesos
Description
-------
The resource provider manager didn't allow operations originating from
operator API calls. For speculatively applied operations, this is
allowed now.
Diffs (updated)
-----
src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
Diff: https://reviews.apache.org/r/70165/diff/3/
Changes: https://reviews.apache.org/r/70165/diff/2-3/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213570
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70165]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 8, 2019, 5:34 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 8, 2019, 5:34 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
>
>
> Diff: https://reviews.apache.org/r/70165/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213560
-----------------------------------------------------------
Thanks for fixing this and adding a test!
I only did a partial review of the test code. I think we should
1) clearly group the different test stages and add comments explaining what we are doing and
2) distinguish `AWAIT` for synchronization from ones use to observe behavior under test (offers with correct resources); printing a message for the latter would be helpful, e.g.,
````
AWAIT_READY(offers) << "Did not observe resource result for operation " << operation;
````
src/tests/storage_local_resource_provider_tests.cpp
Lines 5454 (patched)
<https://reviews.apache.org/r/70165/#comment299527>
Let's add a comment that we use a hierarchical role so we can do a reservation refinement.
src/tests/storage_local_resource_provider_tests.cpp
Lines 5469 (patched)
<https://reviews.apache.org/r/70165/#comment299528>
We usually would write
```
EXPECT_CALL(sched, offerRescinded(_, _))
.Times(4);
```
I am not sure there's much value in adding an expectation _for exactly four calls_ here since below we use dedicated matchers to detect exactly the resources we are interested in; how often offers are rescinded isn't under test here.
src/tests/storage_local_resource_provider_tests.cpp
Lines 5473 (patched)
<https://reviews.apache.org/r/70165/#comment299529>
It would be great if we had some grouping of related actions with a comment explaining what we are doing.
One way to do that would be to write related actions in a separate block
```
// Perform stuff so we can do stuff.
{
// Stuff.
}
```
The group here performs a `CREATE_DISK` and spans the lines 5473-5511.
src/tests/storage_local_resource_provider_tests.cpp
Lines 5513-5514 (patched)
<https://reviews.apache.org/r/70165/#comment299530>
This observes just a precondition and could be left as is.
src/tests/storage_local_resource_provider_tests.cpp
Lines 5570-5571 (patched)
<https://reviews.apache.org/r/70165/#comment299531>
This observes that the operation under test succeeded which should be called out.
- Benjamin Bannier
On March 8, 2019, 2:34 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 8, 2019, 2:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
>
>
> Diff: https://reviews.apache.org/r/70165/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 70165: Fixed operator operation handling with
resource provider resources.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70165/#review213564
-----------------------------------------------------------
If it's possible that we might want to backport this in the future, I'd suggest we split the fix and the tests in two patches :)
- Chun-Hung Hsiao
On March 8, 2019, 1:34 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
>
> (Updated March 8, 2019, 1:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9612
> https://issues.apache.org/jira/browse/MESOS-9612
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f
> src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f
>
>
> Diff: https://reviews.apache.org/r/70165/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>