Re: Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Greg Mann]

> On Jan. 19, 2018, 5:03 p.m., Benjamin Bannier wrote:
> > Thanks for this fix Greg. This will help remove some unsafe boilerplate 
> > from tests. We should probably follow up with a tech-debt ticket removing 
> > unnecessary clock resumes in tests.

Created https://issues.apache.org/jira/browse/MESOS-8466 to look for 
unnecessary calls to `Clock::resume()` in the tests.


- Greg


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


On Jan. 19, 2018, 5:36 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65232/
> ---
> 
> (Updated Jan. 19, 2018, 5:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Because the `cgroups::destroy()` code path makes use of `delay()`,
> the clock must not be paused in order for the destructor of the
> test `Slave` to reliably destroy all remaining containers.
> 
> This patch updates the destructor to check if the clock is paused
> and, if so, resume it before destroying containers.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 
> 
> 
> Diff: https://reviews.apache.org/r/65232/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Greg Mann]

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

(Updated Jan. 19, 2018, 5:36 p.m.)


Review request for mesos, Benjamin Bannier and Gaston Kleiman.


Repository: mesos


Description
---

Because the `cgroups::destroy()` code path makes use of `delay()`,
the clock must not be paused in order for the destructor of the
test `Slave` to reliably destroy all remaining containers.

This patch updates the destructor to check if the clock is paused
and, if so, resume it before destroying containers.


Diffs (updated)
-

  src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 


Diff: https://reviews.apache.org/r/65232/diff/2/

Changes: https://reviews.apache.org/r/65232/diff/1-2/


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Benjamin Bannier]

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


Fix it, then Ship it!




Thanks for this fix Greg. This will help remove some unsafe boilerplate from 
tests. We should probably follow up with a tech-debt ticket removing 
unnecessary clock resumes in tests.


src/tests/cluster.cpp
Lines 117 (patched)


This would be the only symbol from `process` we pull into this scope. Let's 
stick to the current pattern of full names here for local consistency.



src/tests/cluster.cpp
Line 342 (original), 344 (patched)


Suggest to not touch this now, see above.

Here and below in `Master::start`.


- Benjamin Bannier


On Jan. 19, 2018, 1:40 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65232/
> ---
> 
> (Updated Jan. 19, 2018, 1:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Because the `cgroups::destroy()` code path makes use of `delay()`,
> the clock must not be paused in order for the destructor of the
> test `Slave` to reliably destroy all remaining containers.
> 
> This patch updates the destructor to check if the clock is paused
> and, if so, resume it before destroying containers.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 
> 
> 
> Diff: https://reviews.apache.org/r/65232/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Jan. 18, 2018, 4:40 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65232/
> ---
> 
> (Updated Jan. 18, 2018, 4:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Because the `cgroups::destroy()` code path makes use of `delay()`,
> the clock must not be paused in order for the destructor of the
> test `Slave` to reliably destroy all remaining containers.
> 
> This patch updates the destructor to check if the clock is paused
> and, if so, resume it before destroying containers.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 
> 
> 
> Diff: https://reviews.apache.org/r/65232/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Greg Mann]

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

Review request for mesos, Benjamin Bannier and Gaston Kleiman.


Repository: mesos


Description
---

Because the `cgroups::destroy()` code path makes use of `delay()`,
the clock must not be paused in order for the destructor of the
test `Slave` to reliably destroy all remaining containers.

This patch updates the destructor to check if the clock is paused
and, if so, resume it before destroying containers.


Diffs
-

  src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 


Diff: https://reviews.apache.org/r/65232/diff/1/


Testing
---

make check


Thanks,

Greg Mann