> On Jan. 17, 2018, 8:44 a.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 1797 (patched) > > <https://reviews.apache.org/r/65039/diff/2/?file=1936333#file1936333line1797> > > > > This seems unneeded. > > Greg Mann wrote: > If the clock isn't resumed at the end of the test, teardown will not > complete successfully. The destruction of containers in `Slave::~Slave()` in > 'cluster.cpp' requires the clock to be running in order to finish. > > Benjamin Bannier wrote: > That sounds like a bug in `cluster::Slave::~Slave`, and I do not see this > explicit `resume` pattern elsewhere a lot either. > > Adding an explicit `resume` also does not help in cases where an > assertion before the `resume` leads to the rest of the test being skipped. > The only way to make sure it is called is to invoke it in some destructor > RAII style. > > I'd suggest to drop the `resume` here for consistency, and instead fix > the destructor of `~Slave` to make it self-sufficient (either file a ticket > or add a cleanup).
Thanks for pushing on this. After further investigation, it turns out that the `cgroups::destroy()` code path makes use of `delay()`. Thus, the clock cannot be paused in order for code like the tests' `Slave::~Slave()` to reliably destroy containers. In fact, we have other test fixtures like `ContainerizerTest` which already resume/pause the clock when necessary for this reason. I posted a patch to add this behavior to the destructor, and removed the `Clock::resume()` call from this test: https://reviews.apache.org/r/65232/ - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65039/#review195539 ----------------------------------------------------------- On Jan. 18, 2018, 1:57 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65039/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2018, 1:57 a.m.) > > > Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, > and Jie Yu. > > > Bugs: MESOS-8373 > https://issues.apache.org/jira/browse/MESOS-8373 > > > Repository: mesos > > > Description > ------- > > This patch adds > StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation > in order to verify that reconciliation is performed correctly > when an operation is dropped on its way from the master to the > agent. > > > Diffs > ----- > > src/tests/storage_local_resource_provider_tests.cpp > bbfe95e9818f25fdd5405db3ad2fe355e023f743 > > > Diff: https://reviews.apache.org/r/65039/diff/3/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Greg Mann > >
