mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
Date Fri, 17 Apr 2015 20:07:30 GMT


> On April 2, 2015, 7:58 p.m., Vinod Kone wrote:
> > src/tests/master_allocator_tests.cpp, lines 96-104
> > <https://reviews.apache.org/r/31263/diff/7/?file=912106#file912106line96>
> >
> >     While the TearDown() avoids flakiness by ensuring that an allocator process
doesn't exist after a test, it doesn't address cases where a master is restarted with a new
allocator in the same test (e.g., FrameworkReregistersFirst and SlaveReregistersFirst) causing
both old and new allocators to coexist at the same time and talking to the same master.
> >     
> >     I suggest to have methods on the test fixture that creates and deletes allocator
explicitly (can be used by tests that restart master). The TearDown() will still delete any
active allocator. For safety, ensure that only one allocator is active  at any given time.
> >     
> >     Does that make sense?
> >     
> >     ```
> >     template <typename T>
> >     class MasterAllocatorTest : public MesosTest
> >     {
> >     protected: 
> >       MasterAllocatorTest() : allocator(Null) {}
> >       
> >       Try<TestAllocator> create()
> >       {
> >         // We don't support multiple allocators because...
> >         if (allocator != NULL) {
> >           return Error("Multiple active allocators are not supported");
> >         }
> >         
> >         allocator = new TestAllocator(process::Owned<Allocator>(new T));
> >         return *allocator;
> >       }
> >       
> >       void destroy()
> >       {
> >         delete allocator;
> >         allocator = NULL;
> >       }
> >       
> >       virtual void TearDown()
> >       {
> >         destroy();
> >         MeosTest::TearDown();
> >       }
> >     
> >     private:
> >       TestAllocator* allocator;
> >     };
> >     
> >     ```
> 
> Alexander Rukletsov wrote:
>     We used to have a lifecycle method but decided to remove it:
>     be6246a11276074dfb60a892a890b80cb7cd37cd
>     RR: https://reviews.apache.org/r/29927/
>     
>     The two tests you point to are currently the only ones that create an additional
allocator. They both create a new leader instance and appoint the slave instance to the new
leader, and there is no possibility AFAIK to swap allocators in a leader instance. So yes,
there are two active allocators in these tests, but they belong to different leaders.
>     
>     Current design ensures there is only one active allocator in the fixture, but allows
to create more if needed (what those two tests do). The design you propose doesn't prevent
us from creating one more allocator in the test body either.
>     
>     Thoughts?

@Vinod: Just for clarification,
> causing both old and new allocators to coexist at the same time and talking to the same
master.
They do coexist at the same time, but they don't talk to the same master.

My suggestion here is to contain each of the runs in their own lexical scope. Here's what
`FrameworkReregistersFirst` could look like with this approach:
```
TYPED_TEST(MasterAllocatorTest, FrameworkReregistersFirst)
{
  // Components that last both runs.
  MockExecutor exec(DEFAULT_EXECUTOR_ID);

  StandaloneMasterDetector slaveDetector;

  slave::Flags flags = this->CreateSlaveFlags();
  flags.resources = Some("cpus:2;mem:1024");

  Try<PID<Slave> > slave = this->StartSlave(&exec, &slaveDetector,
flags);
  ASSERT_SOME(slave);

  MockScheduler sched;
  StandaloneMasterDetector schedulerDetector;
  TestingMesosSchedulerDriver driver(&sched, &schedulerDetector);

  driver.start();

  /* run1 */ {
    TestAllocator allocator1 = TestAllocator::create<TypeParam>();

    Try<PID<Master>> master1 = this->StartMaster(&allocator1);
    ASSERT_SOME(master1);

    schedulerDetector.appoint(master1.get());

    slaveDetector.appoint(master1.get());

    ...

    this->ShutdownMasters();
  }
  
  /* run2 */ {
    TestAllocator allocator2 = TestAllocator::create<TypeParam>();

    Try<PID<Master>> master2 = this->StartMaster(&allocator2);
    ASSERT_SOME(master2);

    schedulerDetector.appoint(master2.get());

    slaveDetector.appoint(master2.get());

    ...

    this->ShutdownMasters();
  }

  // Shut everything down.
  driver.stop();
  driver.join();

  this->Shutdown();
}  
```

I think the common components, and each of the runs are clearly captured in this approach,
the lifetimes of the objects are well contained within the runs and we could use `allocator`
and `master` in both scopes as well if we felt like it.

The `TestAllocator::create` above is suggested in https://reviews.apache.org/r/31265.
If we find `TestAllocator::create<TypeParam>()` to be too verbose, we could also provide
a `createTestAllocator` alias in `MasterAllocatorTest`:

```
template <typename T>
class MasterAllocatorTest : public MesosTest
{

  ...

  TestAllocator createTestAllocator() const {
    return TestAllocator::create<T>();
  }

  ...
};
```


- Michael


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


On April 15, 2015, 2:18 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 2:18 p.m.)
> 
> 
> Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2160
>     https://issues.apache.org/jira/browse/MESOS-2160
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest
fixture stores the test allocator in turn.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 
>   src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 
>   src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 
>   src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 
>   src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e 
> 
> Diff: https://reviews.apache.org/r/31263/diff/
> 
> 
> Testing
> -------
> 
> make check (Mac OS 10.9.5, CentOS 7.0)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message