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 Bannier <bb...@apache.org> on 2019/05/27 16:32:14 UTC
Review Request 70728: Backed `MockResourceProvider` by a process.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Repository: mesos
Description
-------
We were passing callbacks into `MockResourceProvider` to the HTTP
driver. Since the lifecycle of the callbacks and the mock provider were
decoupled and these callbacks were binding the mock provider instance
the code was not safe as written as the driver could invoke the callback
after the provider had been destructed.
This patch makes sure that the callbacks are defered to a process. We
also dispatch a number of other functions which are strongly coupled to
the lifecycle of the provider. We still do not hide the provider away
completelt so the provider can be mocked in tests.
Diffs
-----
src/tests/api_tests.cpp 2220cecc22778a86f0c29317adf495927e1a900d
src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e
src/tests/mesos.hpp d0c82fa4c1800ed2a1825d563a9462ecd41b77fd
src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
Diff: https://reviews.apache.org/r/70728/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Benjamin Bannier <bb...@apache.org>.
> On June 6, 2019, 12:53 a.m., Chun-Hung Hsiao wrote:
> > src/tests/master_tests.cpp
> > Line 9017 (original), 9017 (patched)
> > <https://reviews.apache.org/r/70728/diff/1/?file=2146840#file2146840line9017>
> >
> > Not yours, but it seems to me that the dequeueing of the `UpdateSlaveMessage` in the master does not causally happen before the `subscribedDefault` call (which sets up the resource provider ID).
https://reviews.apache.org/r/70831/
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review215705
-----------------------------------------------------------
On May 27, 2019, 6:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated May 27, 2019, 6:32 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completelt so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp 2220cecc22778a86f0c29317adf495927e1a900d
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e
> src/tests/mesos.hpp d0c82fa4c1800ed2a1825d563a9462ecd41b77fd
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70728/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review215705
-----------------------------------------------------------
s/completelt/completely/ in the description.
src/tests/master_tests.cpp
Line 9017 (original), 9017 (patched)
<https://reviews.apache.org/r/70728/#comment302518>
Not yours, but it seems to me that the dequeueing of the `UpdateSlaveMessage` in the master does not causally happen before the `subscribedDefault` call (which sets up the resource provider ID).
- Chun-Hung Hsiao
On May 27, 2019, 4:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated May 27, 2019, 4:32 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completelt so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp 2220cecc22778a86f0c29317adf495927e1a900d
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e
> src/tests/mesos.hpp d0c82fa4c1800ed2a1825d563a9462ecd41b77fd
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70728/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review215532
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70728]
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 May 27, 2019, 4:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated May 27, 2019, 4:32 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completelt so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp 2220cecc22778a86f0c29317adf495927e1a900d
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e
> src/tests/mesos.hpp d0c82fa4c1800ed2a1825d563a9462ecd41b77fd
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70728/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Benjamin Bannier <bb...@apache.org>.
> On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote:
> > src/tests/mesos.hpp
> > Lines 3038 (patched)
> > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3041>
> >
> > It seems that I'm wrong about `Self`, which seems to become a type template in a class template. However, I've tested that we can simply use `MockResourceProviderProcess` (whitout any template argument) within this class without introducing a type alias. For example, use `&MockResourceProviderProcess::connectDefault`.
> >
> > That said, please feel free to drop this if you think introducing the `Self` type alias make the code more consistent.
You are right, for uses inside this class we can leave off the template args. I have a hunch that at some point I was required to refer to the type from the outside, but I am either wrong or that use case has gone away. Will remove the type alias altogether.
> On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote:
> > src/tests/mesos.hpp
> > Lines 3151 (patched)
> > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3155>
> >
> > This introduces a coupling beween this test and the implementation of the JWT secret generator being synchoronous.
> >
> > How about the following instead:
> > ```
> > process::Future<Option<std::string>> token = None();
> >
> > #ifdef USE_SSL_SOCKET
> > ...
> >
> > token = secretGenerator.generate(principal)
> > .then([](const Secret& secret) -> Option<std::string> {
> > return secret.value().data();
> > });
> >
> > #endif
> >
> > // TODO(bbannier): Remove the `shared_ptr` once we get C++14.
> > auto detector_ =
> > std::make_shared<process::Owned<EndpointDetector>>(std::move(detector));
> >
> > token.then(defer(self(), [=](const Option<std::string>& token) {
> > driver.reset(new Driver(
> > std::move(*detector_),
> > ...,
> > token));
> >
> > driver->start();
> >
> > return Nothing();
> > }));
> > ```
I like this, thanks for the suggestion; adopted.
> On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote:
> > src/tests/mesos.hpp
> > Lines 3302 (patched)
> > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3326>
> >
> > Do we use `FIXME` in the codebase?
We don't, this was merely a note to myself which I missed to remove (I use `FIXME` instead of `TODO` precisely since we do not use it in the code and it can easily be greped for). Gone now.
> On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote:
> > src/tests/mesos.hpp
> > Lines 3359 (patched)
> > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3383>
> >
> > No need to make it public anymore.
We need to have `process` accessible from the outside so users can interact with the mock. While technically we could make the type alias private I am unsure what that would accomplish but possible future scratched heads. Dropping this, please reopen if I am missing the point.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review216979
-----------------------------------------------------------
On July 31, 2019, 11:31 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated July 31, 2019, 11:31 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9560
> https://issues.apache.org/jira/browse/MESOS-9560
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completely so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a
> src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026
>
>
> Diff: https://reviews.apache.org/r/70728/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review216979
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/mesos.hpp
Lines 3038 (patched)
<https://reviews.apache.org/r/70728/#comment304212>
It seems that I'm wrong about `Self`, which seems to become a type template in a class template. However, I've tested that we can simply use `MockResourceProviderProcess` (whitout any template argument) within this class without introducing a type alias. For example, use `&MockResourceProviderProcess::connectDefault`.
That said, please feel free to drop this if you think introducing the `Self` type alias make the code more consistent.
src/tests/mesos.hpp
Line 3142 (original), 3150 (patched)
<https://reviews.apache.org/r/70728/#comment304213>
s/since //
src/tests/mesos.hpp
Lines 3151 (patched)
<https://reviews.apache.org/r/70728/#comment304214>
This introduces a coupling beween this test and the implementation of the JWT secret generator being synchoronous.
How about the following instead:
```
process::Future<Option<std::string>> token = None();
#ifdef USE_SSL_SOCKET
...
token = secretGenerator.generate(principal)
.then([](const Secret& secret) -> Option<std::string> {
return secret.value().data();
});
#endif
// TODO(bbannier): Remove the `shared_ptr` once we get C++14.
auto detector_ =
std::make_shared<process::Owned<EndpointDetector>>(std::move(detector));
token.then(defer(self(), [=](const Option<std::string>& token) {
driver.reset(new Driver(
std::move(*detector_),
...,
token));
driver->start();
return Nothing();
}));
```
src/tests/mesos.hpp
Lines 3302 (patched)
<https://reviews.apache.org/r/70728/#comment304216>
Do we use `FIXME` in the codebase?
src/tests/mesos.hpp
Lines 3312 (patched)
<https://reviews.apache.org/r/70728/#comment304215>
s/provider_id/providerId/ for naming consistency.
src/tests/mesos.hpp
Lines 3359 (patched)
<https://reviews.apache.org/r/70728/#comment304217>
No need to make it public anymore.
src/tests/mesos.hpp
Line 3350 (original), 3408 (patched)
<https://reviews.apache.org/r/70728/#comment304218>
How about s/Mock/Test/ here and below?
This is more of a personal naming opinion so please feel free to drop it.
src/tests/resource_provider_manager_tests.cpp
Lines 1203 (patched)
<https://reviews.apache.org/r/70728/#comment304219>
s/terminate/stop/ and remove the line break.
src/tests/resource_provider_manager_tests.cpp
Lines 1436 (patched)
<https://reviews.apache.org/r/70728/#comment304220>
Ditto.
- Chun-Hung Hsiao
On July 27, 2019, 7:04 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated July 27, 2019, 7:04 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9560
> https://issues.apache.org/jira/browse/MESOS-9560
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completely so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a
> src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026
>
>
> Diff: https://reviews.apache.org/r/70728/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/
-----------------------------------------------------------
(Updated July 31, 2019, 11:31 a.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Bugs: MESOS-9560
https://issues.apache.org/jira/browse/MESOS-9560
Repository: mesos
Description
-------
We were passing callbacks into `MockResourceProvider` to the HTTP
driver. Since the lifecycle of the callbacks and the mock provider were
decoupled and these callbacks were binding the mock provider instance
the code was not safe as written as the driver could invoke the callback
after the provider had been destructed.
This patch makes sure that the callbacks are defered to a process. We
also dispatch a number of other functions which are strongly coupled to
the lifecycle of the provider. We still do not hide the provider away
completely so the provider can be mocked in tests.
Diffs (updated)
-----
src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510
src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a
src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964
src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026
Diff: https://reviews.apache.org/r/70728/diff/8/
Changes: https://reviews.apache.org/r/70728/diff/7-8/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/
-----------------------------------------------------------
(Updated July 27, 2019, 9:04 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Do not access the process' `info` directly
Bugs: MESOS-9560
https://issues.apache.org/jira/browse/MESOS-9560
Repository: mesos
Description
-------
We were passing callbacks into `MockResourceProvider` to the HTTP
driver. Since the lifecycle of the callbacks and the mock provider were
decoupled and these callbacks were binding the mock provider instance
the code was not safe as written as the driver could invoke the callback
after the provider had been destructed.
This patch makes sure that the callbacks are defered to a process. We
also dispatch a number of other functions which are strongly coupled to
the lifecycle of the provider. We still do not hide the provider away
completely so the provider can be mocked in tests.
Diffs (updated)
-----
src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510
src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a
src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964
src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026
Diff: https://reviews.apache.org/r/70728/diff/7/
Changes: https://reviews.apache.org/r/70728/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Benjamin Bannier <bb...@apache.org>.
> On July 18, 2019, 9:48 p.m., Chun-Hung Hsiao wrote:
> > src/tests/mesos.hpp
> > Lines 3361-3372 (patched)
> > <https://reviews.apache.org/r/70728/diff/2/?file=2148571#file2148571line3386>
> >
> > No need to introduce this type alias in this class template since the class template `MockResourceProviderProcess` is already public.
We use this to bind the `MockResourceProviderProcess`'s template arguments in a single place instead of us having to spell out the full instantiations again and again, e.g., when passed to `dispatch` or elsewhere.
Dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review216741
-----------------------------------------------------------
On May 27, 2019, 6:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated May 27, 2019, 6:32 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9560
> https://issues.apache.org/jira/browse/MESOS-9560
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completely so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a
> src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026
>
>
> Diff: https://reviews.apache.org/r/70728/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70728: Backed `MockResourceProvider` by a process.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70728/#review216741
-----------------------------------------------------------
src/tests/master_tests.cpp
Line 9017 (original), 9017 (patched)
<https://reviews.apache.org/r/70728/#comment303953>
There are two data races here:
1. `updateSlaveMessage` being ready doesn't mean that the `SUBSCRIBED` has been received by the resource provider process.
2. This line and the `subscribed` function accesses `info` in two differen threads without a proper synchronization.
I'd suggest that we have a `Future<ResourceProviderInfo>` or `Future<ResourceProviderID>` or `Future<Subscribed>` in `MockResourceProvider` that is set up in the `subscribed` callback, and we wait for the future here, then avoid accessing `process->info` directly in the remaining of this test. If you don't want a copy, a reference that is set up after the wait is also okay since it would be clear that we have validated the reference. Maybe something like
```
AWAIT_READY(resourceProvider->info());
const ResourceProviderID& resourceProviderId = resourceProvider->info().id();
```
src/tests/master_tests.cpp
Line 9246 (original), 9247 (patched)
<https://reviews.apache.org/r/70728/#comment303955>
Let's not run the actor's member functions in a different context:
```
dispatch(resourceProvider.process, &MockResourceProviderProcess::operationDefault, operation.get());
```
src/tests/master_tests.cpp
Line 9376 (original), 9377 (patched)
<https://reviews.apache.org/r/70728/#comment303956>
We need to have a proper synchronization here to avoid data races. See my comments in `UpdateSlaveMessageWithPendingOffers`.
Also, let's avoid accessing `process->info` directly in the remaining of this test.
src/tests/master_tests.cpp
Line 9472 (original), 9473 (patched)
<https://reviews.apache.org/r/70728/#comment303957>
Let's not run the actor's member functions in a different context:
```
dispatch(resourceProvider.process, &MockResourceProviderProcess::operationDefault, applyOperation.get());
```
src/tests/mesos.hpp
Line 3030 (original), 3041 (patched)
<https://reviews.apache.org/r/70728/#comment303942>
No need to have a type alias here if you use `Self`.
src/tests/mesos.hpp
Line 3037 (original), 3048 (patched)
<https://reviews.apache.org/r/70728/#comment303947>
Not yours, but this template parameter is not used in the class template.
src/tests/mesos.hpp
Lines 3062 (patched)
<https://reviews.apache.org/r/70728/#comment303943>
Let's use `Self::connectDefault` instead. Ditto below.
src/tests/mesos.hpp
Line 3152 (original), 3168 (patched)
<https://reviews.apache.org/r/70728/#comment303949>
s/`MockResourceProviderProcessT`/`Self`/. Ditto below.
src/tests/mesos.hpp
Line 3270 (original), 3269 (patched)
<https://reviews.apache.org/r/70728/#comment303948>
Not yours, but since `Source` can be deduced from `Resoruce`, let's use `Resource::DiskInfo::Source` here and remove the `Source` template parameter above.
src/tests/mesos.hpp
Lines 3322 (patched)
<https://reviews.apache.org/r/70728/#comment303951>
Just a naming suggestion. Since `MockResourceProvider` is no longer a "mock" class (i.e., has no mock function), how about renaming it to `TestResourceProvider`, and the backing process becomes `TestResourceProviderProcess`? This is the pattern I used for `TestDiskProfileServer`.
Feel free to drop this though.
src/tests/mesos.hpp
Lines 3335 (patched)
<https://reviews.apache.org/r/70728/#comment303974>
Let's do `process::terminate(*process)` instead and remove the `terminate` function. See below.
src/tests/mesos.hpp
Lines 3350 (patched)
<https://reviews.apache.org/r/70728/#comment303973>
The use of this `terminate` function in other tests are error prone to me. Instead, let's have a `stop` function in `MockResourceProviderProcess`:
```
void stop()
{
driver.reset();
}
```
And get rid of this function. See my comments in `ResubscribeResourceProvider`.
Also, this function provides a second way to stop the resource provider other than `resourceProvider.reset()`, and we use both in tests. I'd suggesting unify them by removing this function.
As of the name, I'd prefer `stop` over `terminate` to match the `start` call.
src/tests/mesos.hpp
Lines 3361-3372 (patched)
<https://reviews.apache.org/r/70728/#comment303950>
No need to introduce this type alias in this class template since the class template `MockResourceProviderProcess` is already public.
src/tests/operation_reconciliation_tests.cpp
Line 1760 (original), 1760 (patched)
<https://reviews.apache.org/r/70728/#comment303958>
We need to have a proper synchronization here to avoid data races. See my comments in `UpdateSlaveMessageWithPendingOffers`.
src/tests/resource_provider_manager_tests.cpp
Line 1167 (original), 1167 (patched)
<https://reviews.apache.org/r/70728/#comment303959>
We need to have a proper synchronization here to avoid data races. See my comments in `UpdateSlaveMessageWithPendingOffers`.
src/tests/resource_provider_manager_tests.cpp
Line 1195 (original), 1195 (patched)
<https://reviews.apache.org/r/70728/#comment303970>
We're accessing the local variable `resourceProvider` in its process context, which seems error prone. How about having a `MockResourceProviderProcess::stop` function that resets its driver, and invoke it here:
```
Invoke(*resourceProvder->process, &MockResourceProviderProcess::stop)
```
src/tests/resource_provider_manager_tests.cpp
Line 1272 (original), 1272 (patched)
<https://reviews.apache.org/r/70728/#comment303971>
Let's avoid accessing the local variable `resourceProvider` in its process context:
```
Invoke(*resourceProvder->process, &MockResourceProviderProcess::stop)
```
See my comments in `ResubscribeResourceProvider`.
src/tests/resource_provider_manager_tests.cpp
Line 1342 (original), 1342 (patched)
<https://reviews.apache.org/r/70728/#comment303960>
We need to have a proper synchronization here to avoid data races. See my comments in `UpdateSlaveMessageWithPendingOffers`.
src/tests/resource_provider_manager_tests.cpp
Line 1421 (original), 1420 (patched)
<https://reviews.apache.org/r/70728/#comment303972>
Let's avoid accessing the local variable `resourceProvider1` in its process context:
```
Invoke(*resourceProvder1->process, &MockResourceProviderProcess::stop)
```
See my comments in `ResubscribeResourceProvider`.
src/tests/slave_tests.cpp
Line 10835 (original), 10836 (patched)
<https://reviews.apache.org/r/70728/#comment303962>
In this test, `applyOperation` being ready could indicate that `subscribed` has been called, which means that `info` has been set up, so no data race here. However, it would be nice to make these more explicit and avoid direct accesses to `process->info` in different threads. See my comments in `UpdateSlaveMessageWithPendingOffers`.
src/tests/slave_tests.cpp
Lines 10870 (patched)
<https://reviews.apache.org/r/70728/#comment303964>
Let's use `resourceProviderId` here instead.
src/tests/slave_tests.cpp
Line 11256 (original), 11258 (patched)
<https://reviews.apache.org/r/70728/#comment303965>
In this test, `operation` being ready could indicate that `subscribed` has been called, which means that `info` has been set up, so no data race here. However, it would be nice to make these more explicit and avoid direct accesses to `process->info` in the remaining of this test. See my comments in `UpdateSlaveMessageWithPendingOffers`.
src/tests/slave_tests.cpp
Line 11402 (original), 11406 (patched)
<https://reviews.apache.org/r/70728/#comment303967>
s/`CHECK`/`ASSERT_TRUE`/.
We need to have a proper synchronization here to avoid data races. See my comments in `UpdateSlaveMessageWithPendingOffers`.
Also, let's avoid accessing `process->info` directly in the remaining of this test.
src/tests/slave_tests.cpp
Line 11769 (original), 11775 (patched)
<https://reviews.apache.org/r/70728/#comment303975>
Let's make `resourceProvider` an `Owned` or `unique_ptr`, and do `resourceProvider.reset()` instead.
- Chun-Hung Hsiao
On May 27, 2019, 4:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70728/
> -----------------------------------------------------------
>
> (Updated May 27, 2019, 4:32 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9560
> https://issues.apache.org/jira/browse/MESOS-9560
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were passing callbacks into `MockResourceProvider` to the HTTP
> driver. Since the lifecycle of the callbacks and the mock provider were
> decoupled and these callbacks were binding the mock provider instance
> the code was not safe as written as the driver could invoke the callback
> after the provider had been destructed.
>
> This patch makes sure that the callbacks are defered to a process. We
> also dispatch a number of other functions which are strongly coupled to
> the lifecycle of the provider. We still do not hide the provider away
> completely so the provider can be mocked in tests.
>
>
> Diffs
> -----
>
> src/tests/api_tests.cpp af1d215f00c8c2224e807677afb4af2d3521235a
> src/tests/master_slave_reconciliation_tests.cpp 7b6ac50adacc8416b91c0dde55ff7ba46a20515c
> src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e
> src/tests/mesos.hpp d0c82fa4c1800ed2a1825d563a9462ecd41b77fd
> src/tests/operation_reconciliation_tests.cpp eae318da2273faae904f0155e49bb23cdb24f007
> src/tests/resource_provider_manager_tests.cpp 7d48f18e89f046df6c92e52edeef592acfb13b10
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70728/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>