mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 62263: Add test to browse and read in sandbox virtual path.
Date Tue, 19 Sep 2017 22:03:47 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62263/#review185738
-----------------------------------------------------------


Fix it, then Ship it!




Made the tweaks based on the comments to avoid another round of reviewing here, see the diff
below:

```
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index 4428a99d8..949e2d44d 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -7800,8 +7800,12 @@ TEST_F(SlaveTest, IgnoreV0ExecutorIfItReregistersWithoutReconnect)
 
 
 // This test verifies that an executor's latest run directory can
-// be browsed by virtual path.
-TEST_F(SlaveTest, BrowseSandboxByVirtualpath)
+// be browsed via the `/files` endpoint both while the executor is
+// still running and after the executor terminates.
+//
+// Note that we only test the recommended virtual path format:
+//   `/framework/FID/executor/EID/latest`.
+TEST_F(SlaveTest, BrowseExecutorSandboxByVirtualPath)
 {
   master::Flags masterFlags = this->CreateMasterFlags();
   Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
@@ -7809,10 +7813,6 @@ TEST_F(SlaveTest, BrowseSandboxByVirtualpath)
 
   slave::Flags agentFlags = this->CreateSlaveFlags();
 
-  agentFlags.hostname = "localhost";
-  agentFlags.resources = "cpus:4;gpus:0;mem:2048;disk:512;ports:[33000-34000]";
-  agentFlags.attributes = "rack:abc;host:myhost";
-
   MockExecutor exec(DEFAULT_EXECUTOR_ID);
   TestContainerizer containerizer(&exec);
 
@@ -7847,8 +7847,6 @@ TEST_F(SlaveTest, BrowseSandboxByVirtualpath)
 
   AWAIT_READY(offers);
   EXPECT_FALSE(offers->empty());
-  FrameworkID frameworkId = offers->front().framework_id();
-  SlaveID slaveId = offers->front().slave_id();
 
   Resources executorResources = Resources::parse("cpus:0.1;mem:32").get();
   executorResources.allocate("*");
@@ -7880,6 +7878,10 @@ TEST_F(SlaveTest, BrowseSandboxByVirtualpath)
   AWAIT_READY(status);
   EXPECT_EQ(TASK_RUNNING, status->state());
 
+  // Manually inject a file into the sandbox.
+  FrameworkID frameworkId = offers->front().framework_id();
+  SlaveID slaveId = offers->front().slave_id();
+
   const string latestRunPath = paths::getExecutorLatestRunPath(
     agentFlags.work_dir, slaveId, frameworkId, DEFAULT_EXECUTOR_ID);
   EXPECT_TRUE(os::exists(latestRunPath));
@@ -7898,8 +7900,8 @@ TEST_F(SlaveTest, BrowseSandboxByVirtualpath)
         query,
         createBasicAuthHeaders(DEFAULT_CREDENTIAL));
 
-    AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
-    AWAIT_EXPECT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", response);
+    AWAIT_ASSERT_RESPONSE_STATUS_EQ(OK().status, response);
+    AWAIT_ASSERT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", response);
 
     Try<JSON::Array> parse = JSON::parse<JSON::Array>(response->body);
     ASSERT_SOME(parse);
@@ -7916,7 +7918,58 @@ TEST_F(SlaveTest, BrowseSandboxByVirtualpath)
         query,
         createBasicAuthHeaders(DEFAULT_CREDENTIAL));
 
-    AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
+    AWAIT_ASSERT_RESPONSE_STATUS_EQ(OK().status, response);
+
+    JSON::Object expected;
+    expected.values["offset"] = 0;
+    expected.values["data"] = "testing";
+
+    AWAIT_EXPECT_RESPONSE_BODY_EQ(stringify(expected), response);
+  }
+
+  // Now destroy the executor and make sure that the sandbox is
+  // still available. We're sure that the GC won't prune the
+  // sandbox since the clock is paused.
+  Future<TaskStatus> status2;
+  EXPECT_CALL(sched, statusUpdate(&driver, _))
+    .WillOnce(FutureArg<1>(&status2));
+
+  EXPECT_CALL(sched, executorLost(_, _, _, _))
+    .Times(AtMost(1));
+
+  AWAIT_READY(containerizer.destroy(frameworkId, DEFAULT_EXECUTOR_ID));
+  //AWAIT_READY(shutdown);
+
+  AWAIT_READY(status2);
+  EXPECT_EQ(TASK_FAILED, status2->state());
+
+  {
+    const string query = string("path=") + virtualPath;
+    Future<Response> response = process::http::get(
+        files,
+        "browse",
+        query,
+        createBasicAuthHeaders(DEFAULT_CREDENTIAL));
+
+    AWAIT_ASSERT_RESPONSE_STATUS_EQ(OK().status, response);
+    AWAIT_ASSERT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", response);
+
+    Try<JSON::Array> parse = JSON::parse<JSON::Array>(response->body);
+    ASSERT_SOME(parse);
+    EXPECT_NE(0, parse->values.size());
+  }
+
+  {
+    const string query =
+      string("path=") + path::join(virtualPath, "foo.bar") + "&offset=0";
+    process::UPID files("files", slave.get()->pid.address);
+    Future<Response> response = process::http::get(
+        files,
+        "read",
+        query,
+        createBasicAuthHeaders(DEFAULT_CREDENTIAL));
+
+    AWAIT_ASSERT_RESPONSE_STATUS_EQ(OK().status, response);
 
     JSON::Object expected;
     expected.values["offset"] = 0;

```


src/tests/slave_tests.cpp
Lines 7804 (patched)
<https://reviews.apache.org/r/62263/#comment262076>

    s/Sandbox/ExecutorSandbox/
    s/p/P/



src/tests/slave_tests.cpp
Lines 7812-7814 (patched)
<https://reviews.apache.org/r/62263/#comment262077>

    Why did you need this?



src/tests/slave_tests.cpp
Lines 7893-7926 (patched)
<https://reviews.apache.org/r/62263/#comment262082>

    It would be good to also test that these are accessible after the executor terminates.


- Benjamin Mahler


On Sept. 13, 2017, 1:25 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62263/
> -----------------------------------------------------------
> 
> (Updated Sept. 13, 2017, 1:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jason Lai.
> 
> 
> Bugs: MESOS-7899
>     https://issues.apache.org/jira/browse/MESOS-7899
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add test to browse and read in sandbox virtual path.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp 1bdadce4c50cbff958f2be2a4261e130b414acfd 
> 
> 
> Diff: https://reviews.apache.org/r/62263/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message