Changed registrar operations to no longer depend on "strict" flag.
Registrar operations to admit and remove slaves now behave the same way, regardless of the value of the "strict" flag. This commit changes the behavior of these operations by enabling the previous "strict" behavior. For example, removing a slave that is not registered results in an `Error`, rather than silently ignoring the problem. However, the master should never attempt to remove a slave that is not registered, so this is a useful invariant to check. Similarly, attempting to admit a slave that is already admitted will now result in an `Error`; this should only occur when there is a slave ID collision, so again it seems appropriate to produce an error rather than ignoring the problem. Review: https://reviews.apache.org/r/51953/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9fa99618 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9fa99618 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9fa99618 Branch: refs/heads/master Commit: 9fa99618eec85624f44b026804c89be509e18f26 Parents: d9d6f7c Author: Neil Conway <neil.con...@gmail.com> Authored: Mon Sep 19 15:49:13 2016 -0700 Committer: Vinod Kone <vinodk...@gmail.com> Committed: Mon Sep 19 15:49:13 2016 -0700 ---------------------------------------------------------------------- src/master/master.cpp | 2 ++ src/master/master.hpp | 22 ++++++++-------------- src/tests/registrar_tests.cpp | 25 ++++--------------------- 3 files changed, 14 insertions(+), 35 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/9fa99618/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index 20ccd8b..763c5e7 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -7372,6 +7372,8 @@ void Master::_removeSlave( << " from the registrar: " << registrarResult.failure(); } + // Should not happen: the master will only try to remove agents that + // are currently admitted. CHECK(registrarResult.get()) << "Agent " << slave->info.id() << " (" << slave->info.hostname() << ") " << "already removed from the registrar"; http://git-wip-us.apache.org/repos/asf/mesos/blob/9fa99618/src/master/master.hpp ---------------------------------------------------------------------- diff --git a/src/master/master.hpp b/src/master/master.hpp index dbcaa3d..a051742 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -1932,18 +1932,14 @@ protected: virtual Try<bool> perform( Registry* registry, hashset<SlaveID>* slaveIDs, - bool strict) + bool) { // Check if this slave is currently admitted. This should only // happen if there is a slaveID collision, but that is extremely // unlikely in practice: slaveIDs are prefixed with the master ID, // which is a randomly generated UUID. if (slaveIDs->contains(info.id())) { - if (strict) { - return Error("Agent already admitted"); - } else { - return false; // No mutation. - } + return Error("Agent already admitted"); } Registry::Slave* slave = registry->mutable_slaves()->add_slaves(); @@ -1971,7 +1967,7 @@ protected: virtual Try<bool> perform( Registry* registry, hashset<SlaveID>* slaveIDs, - bool strict) + bool) { // As currently implemented, this should not be possible: the // master will only mark slaves unreachable that are currently @@ -2024,7 +2020,7 @@ protected: virtual Try<bool> perform( Registry* registry, hashset<SlaveID>* slaveIDs, - bool strict) + bool) { // A slave might try to reregister that appears in the list of // admitted slaves. This can occur when the master fails over: @@ -2125,7 +2121,7 @@ protected: virtual Try<bool> perform( Registry* registry, hashset<SlaveID>* slaveIDs, - bool strict) + bool) { for (int i = 0; i < registry->slaves().slaves().size(); i++) { const Registry::Slave& slave = registry->slaves().slaves(i); @@ -2136,11 +2132,9 @@ protected: } } - if (strict) { - return Error("Agent not yet admitted"); - } else { - return false; // No mutation. - } + // Should not happen: the master will only try to remove agents + // that are currently admitted. + return Error("Agent not yet admitted"); } private: http://git-wip-us.apache.org/repos/asf/mesos/blob/9fa99618/src/tests/registrar_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/registrar_tests.cpp b/src/tests/registrar_tests.cpp index 928bc66..6c42fd1 100644 --- a/src/tests/registrar_tests.cpp +++ b/src/tests/registrar_tests.cpp @@ -274,12 +274,7 @@ TEST_P(RegistrarTest, Admit) AWAIT_READY(registrar.recover(master)); AWAIT_TRUE(registrar.apply(Owned<Operation>(new AdmitSlave(slave)))); - - if (flags.registry_strict) { - AWAIT_FALSE(registrar.apply(Owned<Operation>(new AdmitSlave(slave)))); - } else { - AWAIT_TRUE(registrar.apply(Owned<Operation>(new AdmitSlave(slave)))); - } + AWAIT_FALSE(registrar.apply(Owned<Operation>(new AdmitSlave(slave)))); } @@ -436,29 +431,17 @@ TEST_P(RegistrarTest, Remove) AWAIT_TRUE(registrar.apply(Owned<Operation>(new RemoveSlave(info1)))); - if (flags.registry_strict) { - AWAIT_FALSE(registrar.apply(Owned<Operation>(new RemoveSlave(info1)))); - } else { - AWAIT_TRUE(registrar.apply(Owned<Operation>(new RemoveSlave(info1)))); - } + AWAIT_FALSE(registrar.apply(Owned<Operation>(new RemoveSlave(info1)))); AWAIT_TRUE(registrar.apply(Owned<Operation>(new AdmitSlave(info1)))); AWAIT_TRUE(registrar.apply(Owned<Operation>(new RemoveSlave(info2)))); - if (flags.registry_strict) { - AWAIT_FALSE(registrar.apply(Owned<Operation>(new RemoveSlave(info2)))); - } else { - AWAIT_TRUE(registrar.apply(Owned<Operation>(new RemoveSlave(info2)))); - } + AWAIT_FALSE(registrar.apply(Owned<Operation>(new RemoveSlave(info2)))); AWAIT_TRUE(registrar.apply(Owned<Operation>(new RemoveSlave(info3)))); - if (flags.registry_strict) { - AWAIT_FALSE(registrar.apply(Owned<Operation>(new RemoveSlave(info3)))); - } else { - AWAIT_TRUE(registrar.apply(Owned<Operation>(new RemoveSlave(info3)))); - } + AWAIT_FALSE(registrar.apply(Owned<Operation>(new RemoveSlave(info3)))); }
