[1/2] mesos git commit: Disallowed HTTP executors to subscribe before containerizer recovery.

[an...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master 700f04002 -> 914ab0f64


Disallowed HTTP executors to subscribe before containerizer recovery.

Previously, it was possible for a HTTP based executor to subscribe
with the agent before the containerizer recovery is done. This
was a problem since calling `containerizer->update()` etc. would
result in a failure.

Review: https://reviews.apache.org/r/52408/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/914ab0f6
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/914ab0f6
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/914ab0f6

Branch: refs/heads/master
Commit: 914ab0f640377cfed9cc8a9dabfa40adec500c0e
Parents: 6b99555
Author: Anand Mazumdar <an...@apache.org>
Authored: Thu Sep 29 16:38:07 2016 -0700
Committer: Anand Mazumdar <an...@apache.org>
Committed: Thu Sep 29 16:48:21 2016 -0700

----------------------------------------------------------------------
 src/slave/http.cpp                    |  5 +++
 src/slave/slave.cpp                   |  4 ++
 src/slave/slave.hpp                   |  5 +++
 src/tests/executor_http_api_tests.cpp | 59 ++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/914ab0f6/src/slave/http.cpp
----------------------------------------------------------------------
diff --git a/src/slave/http.cpp b/src/slave/http.cpp
index ff750e2..bdafe1d 100644
--- a/src/slave/http.cpp
+++ b/src/slave/http.cpp
@@ -435,6 +435,11 @@ string Slave::Http::EXECUTOR_HELP() {
 
 Future<Response> Slave::Http::executor(const Request& request) const
 {
+  if (!slave->recoveryInfo.reconnect) {
+    CHECK(slave->state == RECOVERING);
+    return ServiceUnavailable("Agent has not finished recovery");
+  }
+
   // TODO(anand): Add metrics for rejected requests.
 
   if (request.method != "POST") {

http://git-wip-us.apache.org/repos/asf/mesos/blob/914ab0f6/src/slave/slave.cpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
index 7709def..958a33b 100644
--- a/src/slave/slave.cpp
+++ b/src/slave/slave.cpp
@@ -5171,6 +5171,10 @@ Future<Nothing> Slave::_recoverContainerizer(
 
 Future<Nothing> Slave::_recover()
 {
+  // Alow HTTP based executors to subscribe after the
+  // containerizer recovery is complete.
+  recoveryInfo.reconnect = true;
+
   foreachvalue (Framework* framework, frameworks) {
     foreachvalue (Executor* executor, framework->executors) {
       // Set up callback for executor termination.

http://git-wip-us.apache.org/repos/asf/mesos/blob/914ab0f6/src/slave/slave.hpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp
index 5201e80..9beefa1 100644
--- a/src/slave/slave.hpp
+++ b/src/slave/slave.hpp
@@ -321,6 +321,11 @@ public:
     // Flag to indicate if recovery, including reconciling
     // (i.e., reconnect/kill) with executors is finished.
     process::Promise<Nothing> recovered;
+
+    // Flag to indicate that HTTP based executors can
+    // subscribe with the agent. We allow them to subscribe
+    // after the agent recovers the containerizer.
+    bool reconnect = false;
   } recoveryInfo;
 
   // TODO(benh): Clang requires members to be public in order to take

http://git-wip-us.apache.org/repos/asf/mesos/blob/914ab0f6/src/tests/executor_http_api_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/executor_http_api_tests.cpp b/src/tests/executor_http_api_tests.cpp
index 14a907f..9d3281c 100644
--- a/src/tests/executor_http_api_tests.cpp
+++ b/src/tests/executor_http_api_tests.cpp
@@ -34,9 +34,13 @@
 
 #include "master/master.hpp"
 
+#include "master/detector/standalone.hpp"
+
 #include "tests/containerizer.hpp"
 #include "tests/mesos.hpp"
 
+#include "tests/containerizer/mock_containerizer.hpp"
+
 using mesos::internal::master::Master;
 
 using mesos::internal::recordio::Reader;
@@ -44,6 +48,7 @@ using mesos::internal::recordio::Reader;
 using mesos::internal::slave::Slave;
 
 using mesos::master::detector::MasterDetector;
+using mesos::master::detector::StandaloneMasterDetector;
 
 using mesos::v1::executor::Call;
 using mesos::v1::executor::Event;
@@ -53,6 +58,7 @@ using process::Future;
 using process::Message;
 using process::Owned;
 using process::PID;
+using process::Promise;
 
 using process::http::BadRequest;
 using process::http::MethodNotAllowed;
@@ -60,6 +66,7 @@ using process::http::NotAcceptable;
 using process::http::OK;
 using process::http::Pipe;
 using process::http::Response;
+using process::http::ServiceUnavailable;
 using process::http::UnsupportedMediaType;
 
 using recordio::Decoder;
@@ -706,6 +713,58 @@ TEST_P(ExecutorHttpApiTest, StatusUpdateCallFailedValidation)
 }
 
 
+// This test verifies that the executor cannot subscribe with the agent
+// before it recovers the containerizer.
+TEST_F(ExecutorHttpApiTest, SubscribeBeforeContainerizerRecovery)
+{
+  Try<Owned<cluster::Master>> master = StartMaster();
+  ASSERT_SOME(master);
+
+  MockContainerizer mockContainerizer;
+  StandaloneMasterDetector detector;
+
+  Future<Nothing> recover = FUTURE_DISPATCH(_, &Slave::recover);
+
+  Promise<Nothing> recoveryPromise;
+  EXPECT_CALL(mockContainerizer, recover(_))
+    .WillOnce(Return(recoveryPromise.future()));
+
+  Try<Owned<cluster::Slave>> slave = StartSlave(&detector, &mockContainerizer);
+  ASSERT_SOME(slave);
+
+  // Ensure that the agent has atleast set up HTTP routes upon startup.
+  AWAIT_READY(recover);
+
+  // Send a subscribe call. This should fail with a '503 Service Unavailable'
+  // since the agent hasn't finished recovering the containerizer.
+
+  Call call;
+  call.mutable_framework_id()->CopyFrom(DEFAULT_V1_FRAMEWORK_INFO.id());
+  call.mutable_executor_id()->CopyFrom(DEFAULT_V1_EXECUTOR_ID);
+
+  call.set_type(Call::SUBSCRIBE);
+
+  call.mutable_subscribe();
+
+  process::http::Headers headers;
+  headers["Accept"] = APPLICATION_JSON;
+
+  Future<Response> response = process::http::post(
+        slave.get()->pid,
+        "api/v1/executor",
+        headers,
+        serialize(ContentType::JSON, call),
+        APPLICATION_JSON);
+
+  AWAIT_EXPECT_RESPONSE_STATUS_EQ(ServiceUnavailable().status, response);
+
+  // The destructor of `cluster::Slave` will try to clean up any
+  // remaining containers by inspecting the result of `containers()`.
+  EXPECT_CALL(mockContainerizer, containers())
+    .WillRepeatedly(Return(hashset<ContainerID>()));
+}
+
+
 // This test verifies if the executor is able to receive a Subscribed
 // event in response to a Subscribe call request.
 TEST_P(ExecutorHttpApiTest, Subscribe)

[2/2] mesos git commit: Added `RecoveryInfo` struct to the agent.

[an...@apache.org]

Added `RecoveryInfo` struct to the agent.

This struct would container all the recovery related metadata
on the agent from now on. Eventually, we would add component
specific recovery information to this struct e.g, the executors
can now subscribe again with the agent etc.

Review: https://reviews.apache.org/r/52407/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/6b99555f
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/6b99555f
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/6b99555f

Branch: refs/heads/master
Commit: 6b99555fa808eb32e32c3624704d0971568ca795
Parents: 700f040
Author: Anand Mazumdar <an...@apache.org>
Authored: Thu Sep 29 16:37:53 2016 -0700
Committer: Anand Mazumdar <an...@apache.org>
Committed: Thu Sep 29 16:48:21 2016 -0700

----------------------------------------------------------------------
 src/slave/slave.cpp |  6 +++---
 src/slave/slave.hpp | 12 ++++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/6b99555f/src/slave/slave.cpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
index 46bc8b3..7709def 100644
--- a/src/slave/slave.cpp
+++ b/src/slave/slave.cpp
@@ -3590,7 +3590,7 @@ void Slave::reregisterExecutorTimeout()
   }
 
   // Signal the end of recovery.
-  recovered.set(Nothing());
+  recoveryInfo.recovered.set(Nothing());
 }
 
 
@@ -5230,7 +5230,7 @@ Future<Nothing> Slave::_recover()
     // We set 'recovered' flag inside reregisterExecutorTimeout(),
     // so that when the slave re-registers with master it can
     // correctly inform the master about the launched tasks.
-    return recovered.future();
+    return recoveryInfo.recovered.future();
   }
 
   return Nothing();
@@ -5331,7 +5331,7 @@ void Slave::__recover(const Future<Nothing>& future)
     // doesn't happen within a timeout.
   }
 
-  recovered.set(Nothing()); // Signal recovery.
+  recoveryInfo.recovered.set(Nothing()); // Signal recovery.
 }
 
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/6b99555f/src/slave/slave.hpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp
index 7b414d0..5201e80 100644
--- a/src/slave/slave.hpp
+++ b/src/slave/slave.hpp
@@ -315,6 +315,14 @@ public:
     TERMINATING,  // Slave is shutting down.
   } state;
 
+  // Describes information about agent recovery.
+  struct RecoveryInfo
+  {
+    // Flag to indicate if recovery, including reconciling
+    // (i.e., reconnect/kill) with executors is finished.
+    process::Promise<Nothing> recovered;
+  } recoveryInfo;
+
   // TODO(benh): Clang requires members to be public in order to take
   // their address which we do in tests (for things like
   // FUTURE_DISPATCH).
@@ -728,10 +736,6 @@ private:
   // the master.
   process::Timer pingTimer;
 
-  // Flag to indicate if recovery, including reconciling (i.e., reconnect/kill)
-  // with executors is finished.
-  process::Promise<Nothing> recovered;
-
   // Root meta directory containing checkpointed data.
   const std::string metaDir;