This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new abf0c7a Moved framework update out of
`connectAndActivateRecoveredFramework`.
abf0c7a is described below
commit abf0c7ab8ae94b06349fdbb74c4e76e78d5c371d
Author: Andrei Sekretenko <asekrete...@apache.org>
AuthorDate: Fri Sep 4 17:04:25 2020 +0200
Moved framework update out of `connectAndActivateRecoveredFramework`.
This patch consolidates updating framework in the Subscribe call
by moving `updateFramework()` invocation from
`connectAndActivateRecoveredFramework()` into `Master::_subscribe()`.
Review: https://reviews.apache.org/r/72838
---
src/master/master.cpp | 71 ++++++++++++++++++++-------------------------------
src/master/master.hpp | 4 +--
2 files changed, 29 insertions(+), 46 deletions(-)
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 5b5d5c3..e99cc6b 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -2823,12 +2823,11 @@ void Master::_subscribe(
framework->metrics.incrementCall(scheduler::Call::SUBSCRIBE);
+ updateFramework(framework, frameworkInfo, std::move(options));
+
if (!framework->recovered()) {
// The framework has previously been registered with this master;
// it may or may not currently be connected.
-
- updateFramework(framework, frameworkInfo, std::move(options));
-
framework->reregisteredTime = Clock::now();
// Always failover the old framework connection. See MESOS-4712 for
details.
@@ -2836,12 +2835,7 @@ void Master::_subscribe(
} else {
// The framework has not yet reregistered after master failover.
connectAndActivateRecoveredFramework(
- framework,
- frameworkInfo,
- None(),
- http,
- objectApprovers.get(),
- std::move(options));
+ framework, None(), http, objectApprovers.get());
}
// TODO(asekretenko): Consider avoiding to broadcast `FrameworkInfo` to
agents
@@ -3123,32 +3117,32 @@ void Master::_subscribe(
return;
}
+ // Using the "force" field of the scheduler allows us to reject a
+ // scheduler that got partitioned but didn't die (in ZooKeeper
+ // speak this means didn't lose their session) and then
+ // eventually tried to connect to this master even though
+ // another instance of their scheduler has reconnected.
+
+ // Test that the scheduler trying to subscribe with a new PID sets `force`.
+ if (!framework->recovered() && framework->pid() != from && !force) {
+ LOG(ERROR) << "Disallowing subscription attempt of"
+ << " framework " << *framework
+ << " because it is not expected from " << from;
+
+ FrameworkErrorMessage message;
+ message.set_message("Framework failed over");
+ send(from, message);
+ return;
+ }
+
+ // It is now safe to update the framework fields since the request is now
+ // guaranteed to be successful. We use the fields passed in during
+ // re-registration.
+ updateFramework(framework, frameworkInfo, std::move(options));
+
if (!framework->recovered()) {
// The framework has previously been registered with this master;
// it may or may not currently be connected.
- //
- // Using the "force" field of the scheduler allows us to keep a
- // scheduler that got partitioned but didn't die (in ZooKeeper
- // speak this means didn't lose their session) and then
- // eventually tried to connect to this master even though
- // another instance of their scheduler has reconnected.
-
- // Test for the error case first.
- if ((framework->pid() != from) && !force) {
- LOG(ERROR) << "Disallowing subscription attempt of"
- << " framework " << *framework
- << " because it is not expected from " << from;
-
- FrameworkErrorMessage message;
- message.set_message("Framework failed over");
- send(from, message);
- return;
- }
-
- // It is now safe to update the framework fields since the request is now
- // guaranteed to be successful. We use the fields passed in during
- // re-registration.
- updateFramework(framework, frameworkInfo, std::move(options));
framework->reregisteredTime = Clock::now();
@@ -3207,12 +3201,7 @@ void Master::_subscribe(
} else {
// The framework has not yet reregistered after master failover.
connectAndActivateRecoveredFramework(
- framework,
- frameworkInfo,
- from,
- None(),
- objectApprovers.get(),
- std::move(options));
+ framework, from, None(), objectApprovers.get());
}
// TODO(asekretenko): Consider avoiding to broadcast `FrameworkInfo` and
@@ -10136,11 +10125,9 @@ void Master::recoverFramework(const FrameworkInfo&
info)
void Master::connectAndActivateRecoveredFramework(
Framework* framework,
- const FrameworkInfo& frameworkInfo,
const Option<UPID>& pid,
const Option<StreamingHttpConnection<v1::scheduler::Event>>& http,
- const Owned<ObjectApprovers>& objectApprovers,
- ::mesos::allocator::FrameworkOptions&& allocatorOptions)
+ const Owned<ObjectApprovers>& objectApprovers)
{
// Exactly one of `pid` or `http` must be provided.
CHECK(pid.isSome() != http.isSome());
@@ -10152,8 +10139,6 @@ void Master::connectAndActivateRecoveredFramework(
CHECK(framework->pid().isNone());
CHECK(framework->http().isNone());
- updateFramework(framework, frameworkInfo, std::move(allocatorOptions));
-
// Updating `registeredTime` here is debatable: ideally,
// `registeredTime` would be the time at which the framework first
// registered with the master. However, we cannot determine this
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 3d1d472..8c2f56a 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -629,11 +629,9 @@ protected:
// Exactly one of `newPid` or `http` must be provided.
void connectAndActivateRecoveredFramework(
Framework* framework,
- const FrameworkInfo& frameworkInfo,
const Option<process::UPID>& pid,
const Option<StreamingHttpConnection<v1::scheduler::Event>>& http,
- const process::Owned<ObjectApprovers>& objectApprovers,
- ::mesos::allocator::FrameworkOptions&& allocatorOptions);
+ const process::Owned<ObjectApprovers>& objectApprovers);
// Replace the scheduler for a framework with a new process ID, in
// the event of a scheduler failover.
