Removed the timeout from the offer filter in the allocator. Without the timeout, we rely on filter expiration only. This guarantees that filter removal is scheduled after `allocate()` if the allocator is backlogged given default parameters are used. Additionally we ensure the filter timeout is at least as big as the allocation interval.
Review: https://reviews.apache.org/r/42355/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/447d814a Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/447d814a Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/447d814a Branch: refs/heads/master Commit: 447d814ac80e67f30a0ffe2ee6047d85dc8fc383 Parents: f5f7670 Author: Alexander Rukletsov <[email protected]> Authored: Thu Jan 21 23:17:22 2016 -0800 Committer: Benjamin Mahler <[email protected]> Committed: Thu Jan 21 23:38:57 2016 -0800 ---------------------------------------------------------------------- src/master/allocator/mesos/hierarchical.cpp | 38 ++++++++++++------------ src/tests/hierarchical_allocator_tests.cpp | 9 ++++-- 2 files changed, 25 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/447d814a/src/master/allocator/mesos/hierarchical.cpp ---------------------------------------------------------------------- diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index 53d8784..6215e2b 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -60,10 +60,7 @@ public: class RefusedOfferFilter : public OfferFilter { public: - RefusedOfferFilter( - const Resources& _resources, - const Timeout& _timeout) - : resources(_resources), timeout(_timeout) {} + RefusedOfferFilter(const Resources& _resources) : resources(_resources) {} virtual bool filter(const Resources& _resources) { @@ -72,12 +69,10 @@ public: // more revocable resources only or non-revocable resources only, // but currently the filter only expires if there is more of both // revocable and non-revocable resources. - return resources.contains(_resources) && // Refused resources are superset. - timeout.remaining() > Seconds(0); + return resources.contains(_resources); // Refused resources are superset. } const Resources resources; - const Timeout timeout; }; @@ -925,11 +920,8 @@ void HierarchicalAllocatorProcess::recoverResources( << " filtered slave " << slaveId << " for " << seconds.get(); - // Create a new filter and delay its expiration. - OfferFilter* offerFilter = new RefusedOfferFilter( - resources, - Timeout::in(seconds.get())); - + // Create a new filter. + OfferFilter* offerFilter = new RefusedOfferFilter(resources); frameworks[frameworkId].offerFilters[slaveId].insert(offerFilter); // We need to disambiguate the function call to pick the correct @@ -939,13 +931,21 @@ void HierarchicalAllocatorProcess::recoverResources( const SlaveID&, OfferFilter*) = &Self::expire; - delay( - seconds.get(), - self(), - expireOffer, - frameworkId, - slaveId, - offerFilter); + // Expire the filter after both an `allocationInterval` and the + // `timeout` have elapsed. This ensures that the filter does not + // expire before we perform the next allocation for this agent, + // see MESOS-4302 for more information. + // + // TODO(alexr): If we allocated upon resource recovery + // (MESOS-3078), we would not need to increase the timeout here. + Duration timeout = std::max(allocationInterval, seconds.get()); + + delay(timeout, + self(), + expireOffer, + frameworkId, + slaveId, + offerFilter); } } http://git-wip-us.apache.org/repos/asf/mesos/blob/447d814a/src/tests/hierarchical_allocator_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/hierarchical_allocator_tests.cpp b/src/tests/hierarchical_allocator_tests.cpp index 9537121..516c5cf 100644 --- a/src/tests/hierarchical_allocator_tests.cpp +++ b/src/tests/hierarchical_allocator_tests.cpp @@ -1281,12 +1281,15 @@ TEST_F(HierarchicalAllocatorTest, QuotaProvidesQuarantee) // general case, because an allocator may be complex enough to postpone // decisions beyond its allocation cycle. - // Now advance the clock to make sure the filter is expired. The next - // and only allocation should be the declined resources offered to the - // quota'ed role. + // Now advance the clock to make sure the filter is expired and removed. Clock::advance(Duration::create(filter5s.refuse_seconds()).get()); Clock::settle(); + // Trigger the next periodic allocation. It should offer the previously + // declined resources to the quota'ed role. + Clock::advance(flags.allocation_interval); + Clock::settle(); + allocation = allocations.get(); AWAIT_READY(allocation); EXPECT_EQ(framework1.id(), allocation.get().frameworkId);
