Fixed a crash on the master upon receiving an invalid inverse offer. The erroneous invariant check for `slaveId` can be trigerred when the master accepts an invalid inverse offer or when the inverse offer has been already rescinded.
Review: https://reviews.apache.org/r/56587/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/fabca9fa Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/fabca9fa Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/fabca9fa Branch: refs/heads/1.1.x Commit: fabca9fa50c3eaeb6fbad19a25e65804ec75fa2b Parents: 0ff757e Author: Anand Mazumdar <[email protected]> Authored: Wed Feb 15 11:56:37 2017 -0800 Committer: Alexander Rukletsov <[email protected]> Committed: Wed Apr 26 14:39:40 2017 +0200 ---------------------------------------------------------------------- src/master/master.cpp | 20 +++------- src/tests/master_maintenance_tests.cpp | 57 +++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/fabca9fa/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index 66ccefa..ecd035a 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -4436,27 +4436,26 @@ void Master::acceptInverseOffers( { CHECK_NOTNULL(framework); - Option<Error> error = None(); + Option<Error> error; if (accept.inverse_offer_ids().size() == 0) { error = Error("No inverse offers specified"); } else { + LOG(INFO) << "Processing ACCEPT_INVERSE_OFFERS call for inverse offers: " + << accept.inverse_offer_ids() << " for framework " << *framework; + // Validate the inverse offers. error = validation::offer::validateInverseOffers( accept.inverse_offer_ids(), this, framework); - Option<SlaveID> slaveId; - // Update each inverse offer in the allocator with the accept and // filter. + // TODO(anand): Notify the framework if some of the offers were invalid. foreach (const OfferID& offerId, accept.inverse_offer_ids()) { InverseOffer* inverseOffer = getInverseOffer(offerId); if (inverseOffer != nullptr) { - CHECK(inverseOffer->has_slave_id()); - slaveId = inverseOffer->slave_id(); - mesos::allocator::InverseOfferStatus status; status.set_status(mesos::allocator::InverseOfferStatus::ACCEPT); status.mutable_framework_id()->CopyFrom(inverseOffer->framework_id()); @@ -4480,15 +4479,6 @@ void Master::acceptInverseOffers( LOG(WARNING) << "Ignoring accept of inverse offer " << offerId << " since it is no longer valid"; } - - CHECK_SOME(slaveId); - Slave* slave = slaves.registered.get(slaveId.get()); - CHECK_NOTNULL(slave); - - LOG(INFO) - << "Processing ACCEPT_INVERSE_OFFERS call for inverse offers: " - << accept.inverse_offer_ids() << " on slave " << *slave - << " for framework " << *framework; } if (error.isSome()) { http://git-wip-us.apache.org/repos/asf/mesos/blob/fabca9fa/src/tests/master_maintenance_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_maintenance_tests.cpp b/src/tests/master_maintenance_tests.cpp index 6917272..1d8e950 100644 --- a/src/tests/master_maintenance_tests.cpp +++ b/src/tests/master_maintenance_tests.cpp @@ -72,9 +72,11 @@ using mesos::v1::scheduler::Mesos; using process::Clock; using process::Future; +using process::Message; using process::Owned; using process::PID; using process::Time; +using process::UPID; using process::http::BadRequest; using process::http::OK; @@ -90,6 +92,7 @@ using std::vector; using testing::AtMost; using testing::DoAll; +using testing::Eq; using testing::Not; namespace mesos { @@ -1828,6 +1831,60 @@ TEST_F(MasterMaintenanceTest, EndpointsBadAuthentication) } } + +// This test verifies that the Mesos master does not crash while +// processing an invalid inverse offer (See MESOS-7119). +TEST_F(MasterMaintenanceTest, AcceptInvalidInverseOffer) +{ + master::Flags masterFlags = CreateMasterFlags(); + + Try<Owned<cluster::Master>> master = StartMaster(masterFlags); + ASSERT_SOME(master); + + MockScheduler sched; + MesosSchedulerDriver driver( + &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL); + + EXPECT_CALL(sched, registered(&driver, _, _)); + + Future<Message> frameworkRegisteredMessage = + FUTURE_MESSAGE(Eq(FrameworkRegisteredMessage().GetTypeName()), _, _); + + driver.start(); + + AWAIT_READY(frameworkRegisteredMessage); + UPID frameworkPid = frameworkRegisteredMessage.get().to; + + FrameworkRegisteredMessage message; + ASSERT_TRUE(message.ParseFromString(frameworkRegisteredMessage.get().body)); + + FrameworkID frameworkId = message.framework_id(); + + Future<mesos::scheduler::Call> acceptInverseOffersCall = FUTURE_CALL( + mesos::scheduler::Call(), + mesos::scheduler::Call::ACCEPT_INVERSE_OFFERS, + _, + _); + + { + mesos::scheduler::Call call; + call.mutable_framework_id()->CopyFrom(frameworkId); + call.set_type(mesos::scheduler::Call::ACCEPT_INVERSE_OFFERS); + + mesos::scheduler::Call::AcceptInverseOffers* accept = + call.mutable_accept_inverse_offers(); + + accept->add_inverse_offer_ids()->set_value("invalid-inverse-offer"); + + process::post(frameworkPid, master.get()->pid, call); + } + + AWAIT_READY(acceptInverseOffersCall); + + driver.stop(); + driver.join(); +} + } // namespace tests { } // namespace internal { } // namespace mesos {
