[xray] Basic task reconstruction mechanism #2526

stephanie-wang · 2018-07-31T18:21:36Z

What do these changes do?

This implements basic task reconstruction in raylet. There are two parts to this PR:

Reconstruction suppression through the TaskReconstructionLog. This prevents two raylets from reconstructing the same task if they decide simultaneously (via the logic in [xray] Implement task lease table, logic for deciding when to reconstruct a task #2497) that reconstruction is necessary.
Task resubmission once a raylet becomes responsible for reconstructing a task.

Reconstruction is quite slow in this PR, especially for long chains of dependent tasks. This is mainly due to the lease table mechanism, where nodes may wait too long before trying to reconstruct a task. There are two ways to improve this:

Expire entries in the lease table using Redis PEXPIRE. This is a WIP and I may include it in this PR.
Introduce a "fast path" for reconstructing dependencies of a re-executed task. Normally, we wait for an initial timeout before checking whether a task requires reconstruction. However, if a task requires reconstruction, then it's likely that its dependencies also require reconstruction. In this case, we could skip the initial timeout before checking the GCS to see whether reconstruction is necessary (e.g., if the object has been evicted).

Since handling failures of other raylets is probably not yet complete in master, this only turns back on Python tests for reconstructing evicted objects.

AmplabJenkins · 2018-07-31T19:47:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7063/
Test PASSed.

robertnishihara · 2018-07-31T20:07:40Z

src/ray/gcs/tables.h

+    // since the lease entry itself contains the expiration period. In the
+    // worst case, if the command fails, then a client that looks up the lease
+    // entry will overestimate the expiration time.
+    std::vector<std::string> args = {"PEXPIRE", "TASK_LEASE" + id.binary(),


Instead of hardcoding TASK_LEASE, we can do something like EnumNameTablePrefix(TablePrefix::TASK_LEASE).

Oops, thanks :)

robertnishihara · 2018-07-31T20:08:41Z

src/ray/gcs/tables.h

+             const WriteCallback &done) override {
+    RAY_RETURN_NOT_OK((Table<TaskID, TaskLeaseData>::Add(job_id, id, data, done)));
+    // Mark the entry for expiration in Redis. It's okay if this command fails
+    // since the lease entry itself contains the expiration period. In the


Hm, is it actually ok if this fails? E.g., if it fails, then it will fail to extend the previous expiration, and so the key could actually expire too early, right?

According to Redis documentation:

The timeout will only be cleared by commands that delete or overwrite the contents of the key, including DEL, SET, GETSET and all the *STORE commands.

I believe that the Add command in the previous line will overwrite the contents.

robertnishihara · 2018-07-31T20:12:31Z

src/ray/raylet/task_spec.h

@@ -82,6 +82,8 @@ class TaskSpecification {
  /// \param string A serialized task specification flatbuffer.
  TaskSpecification(const flatbuffers::String &string);

+  TaskSpecification(const std::string &string);


Please document, in particular it's worth saying that this requires the string to be in scope for at least as long as the TaskSpecification, right?

I'll document. I don't think that is true though, since the constructor performs a copy of the string data.

robertnishihara · 2018-07-31T20:13:33Z

src/ray/raylet/reconstruction_policy_test.cc

+  // reconstruction first. This should suppress this node's first attempt at
+  // reconstruction.
+  auto task_reconstruction_data = std::make_shared<TaskReconstructionDataT>();
+  task_reconstruction_data->node_manager_id = ClientID::from_random().hex();


All UniqueIDs stored in flatbuffer objects should be binary not hex.

robertnishihara · 2018-07-31T20:16:28Z

src/ray/raylet/reconstruction_policy.h

@@ -70,6 +71,8 @@ class ReconstructionPolicy : public ReconstructionPolicyInterface {
  /// this timeout, then objects that the task creates may be reconstructed.
  void HandleTaskLeaseNotification(const TaskID &task_id, int64_t lease_timeout_ms);

+  void HandleReconstructionLogAppend(const TaskID &task_id, bool success);


robertnishihara · 2018-07-31T20:18:58Z

src/ray/raylet/node_manager.cc

+      });
+}
+
+void NodeManager::ResubmitTask(const Task &task) {


If this method is called, then we are guaranteed that task is currently not in local_queues_, right?

We are also guaranteed that this node manager has the task lease, right?

Might be helpful to document some of these constraints/assumptions.

Yes, task should not be in local_queues_. If it is, then there is a spurious reconstruction.

The node manager will acquire the task lease once it calls TaskPending, which should happen once the task is accepted by the node (e.g., in EnqueuePlaceableTask).

Thanks, I'll document that.

robertnishihara · 2018-07-31T20:21:36Z

src/ray/raylet/node_manager.cc

+      [this](ray::gcs::AsyncGcsClient *client, const TaskID &task_id) {
+        const Task &task = lineage_cache_.GetTask(task_id);
+        ResubmitTask(task);
+      });


There are two callbacks here. Can you document what circumstances lead to each of the two callbacks firing?

Yep, thanks!

AmplabJenkins · 2018-07-31T21:21:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7067/
Test FAILed.

AmplabJenkins · 2018-07-31T21:28:58Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7066/
Test FAILed.

AmplabJenkins · 2018-07-31T22:59:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7068/
Test FAILed.

AmplabJenkins · 2018-07-31T23:02:06Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7070/
Test PASSed.

AmplabJenkins · 2018-08-01T00:16:50Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7075/
Test FAILed.

AmplabJenkins · 2018-08-01T08:06:15Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7096/
Test FAILed.

AmplabJenkins · 2018-08-01T18:43:18Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7105/
Test FAILed.

AmplabJenkins · 2018-08-01T22:15:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7114/
Test FAILed.

AmplabJenkins · 2018-08-02T00:41:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7123/
Test FAILed.

robertnishihara · 2018-08-02T00:43:32Z

jenkins, retest this please

AmplabJenkins · 2018-08-02T01:26:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7126/
Test FAILed.

robertnishihara · 2018-08-02T02:04:05Z

jenkins, retest this please

AmplabJenkins · 2018-08-02T03:22:06Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7130/
Test PASSed.

raulchen · 2018-08-02T12:49:34Z

src/ray/gcs/tables.h

+
+  Status Add(const JobID &job_id, const TaskID &id, std::shared_ptr<TaskLeaseDataT> &data,
+             const WriteCallback &done) override {
+    RAY_RETURN_NOT_OK((Table<TaskID, TaskLeaseData>::Add(job_id, id, data, done)));


nit: there's a pair of unnecessary parentheses.

Actually, I need the parentheses for it to build for some reason...I don't know exactly why, but the compiler seems to think that there are 2 arguments to the macro, maybe because of the templated argument?

Is the reason that you have to add Table<TaskID, TaskLeaseData>:: because the two methods have the same name? Would that go away if we just change this method name from Add to something else like UpdateTaskLease?

Just googled this, the reason is because the macro processor doesn't know about template, thus recognizes Table<TaskID as a parameter.
Someone suggests this way:

#define COMMA , RAY_RETURN_NOT_OK(Table<TaskID COMMA TaskLeaseData>::(...))

It works. But I think adding an extra pair of parentheses is easier. Or as @robertnishihara suggested, use another function name.

Sounds good!

I decided against changing the function name since I believe it's nicer to have the TaskLeaseTable still implement the Table<TaskID, TaskLeaseData> interface.

zhijunfu · 2018-08-06T15:51:14Z

src/ray/raylet/lineage_cache.cc

@@ -436,6 +436,13 @@ void LineageCache::HandleEntryCommitted(const TaskID &task_id) {
  }
 }

+const Task LineageCache::GetTask(const TaskID &task_id) const {
+  auto entries = lineage_.GetEntries();


auto -> auto& ?

zhijunfu · 2018-08-06T15:51:41Z

src/ray/raylet/lineage_cache.cc

@@ -436,6 +436,13 @@ void LineageCache::HandleEntryCommitted(const TaskID &task_id) {
  }
 }

+const Task LineageCache::GetTask(const TaskID &task_id) const {


const Task & ?

zhijunfu · 2018-08-06T15:54:53Z

src/ray/raylet/node_manager.cc

+}
+
+void NodeManager::ResubmitTask(const Task &task) {
+  // Actor reconstruction is turned off by default right now. If this is an


just curious: is this a stage approach - aka. actor reconstruction will be enabled later?

Yes, we're still thinking about how exactly actor reconstruction should be exposed to the user. I believe the current plan is to have actor reconstruction enabled by a flag from the frontend.

zhijunfu · 2018-08-06T15:55:14Z

src/ray/raylet/node_manager.cc

+        // The task was not in the GCS task table. It must therefore be in the
+        // lineage cache. Use a copy of the cached task spec to re-execute the
+        // task.
+        const Task task = lineage_cache_.GetTask(task_id);


const auto & ?

I actually mean to copy the task here in case the lineage cache gets modified while resubmitting the task for execution.

AmplabJenkins · 2018-08-06T19:12:48Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7254/
Test FAILed.

AmplabJenkins · 2018-08-08T22:46:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7352/
Test PASSed.

AmplabJenkins · 2018-08-08T22:51:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7353/
Test PASSed.

…tionLog

jovany-wang · 2018-08-09T01:58:07Z

src/ray/raylet/node_manager.cc

-        const Task task = lineage_cache_.GetTask(task_id);
-        ResubmitTask(task);
+        // lineage cache.
+        if (!lineage_cache_.ContainsTask(task_id)) {


I think this is not the right way to solve it.

I left a TODO explaining that we should revert this change once #2608 is fixed. I don't think we should block this PR on #2608 though.

@stephanie-wang That's a nice solution also.
btw, does it still hang after ur this change?

Nope! Although there are many error messages (and will be for any job with tasks that take some time to complete) logged.

AmplabJenkins · 2018-08-09T02:16:16Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7367/
Test PASSed.

AmplabJenkins · 2018-08-09T06:37:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/7375/
Test FAILed.

stephanie-wang requested review from robertnishihara and pcmoritz July 31, 2018 18:21

stephanie-wang force-pushed the xray-reconstruction branch from 1fc636c to 801d4b6 Compare July 31, 2018 19:54

robertnishihara reviewed Jul 31, 2018

View reviewed changes

stephanie-wang changed the title ~~[WIP][xray] Basic task reconstruction mechanism~~ [xray] Basic task reconstruction mechanism Jul 31, 2018

This was referenced Jul 31, 2018

Reconstruction triggered infinitely on infeasible/placeable tasks #2527

Closed

[Java Worker] Support raylet on Java #2479

Merged

robertnishihara force-pushed the xray-reconstruction branch from 1ab9b6c to a8288d2 Compare August 1, 2018 07:24

raulchen reviewed Aug 2, 2018

View reviewed changes

pcmoritz mentioned this pull request Aug 2, 2018

Fix potential boost error message corruption #2548

Closed

zhijunfu reviewed Aug 6, 2018

View reviewed changes

stephanie-wang force-pushed the xray-reconstruction branch from 2c02ca9 to 9fb8f1e Compare August 6, 2018 20:03

stephanie-wang force-pushed the xray-reconstruction branch from e9f5905 to 351e42d Compare August 8, 2018 21:20

stephanie-wang mentioned this pull request Aug 8, 2018

Java and Python UniqueID generation does not match #2608

Closed

stephanie-wang and others added 19 commits August 8, 2018 17:45

Re-execute tasks

5f55166

Suppress simultaneous reconstruction attempts with the TaskReconstruc…

6995e47

…tionLog

Add unit test for TaskReconstructionLog

d2f5876

Handle reconstruction in the node manager

3db0b93

Turn eviction tests back on

45012bc

Expire task lease entries in redis

01289a5

Address comments

31c8867

Check return status

709eecc

check status and unused lambda capture

65331cf

Copy the task from the lineage cache

ea028d5

Fix access of erased iterator in ReconstructionPolicy

81653ae

lint

d205925

Use strerror instead of boost error_code.message()

e8be7a3

Bug fix.

33b17df

Fix

fef5c3e

merge

11793ce

Add set -e -x to java/test.sh.

efe98a0

Change fatal check for task not in lineage cache to a warning

16872d4

oops

bea11c4

stephanie-wang force-pushed the xray-reconstruction branch from 351e42d to bea11c4 Compare August 9, 2018 00:45

jovany-wang reviewed Aug 9, 2018

View reviewed changes

Merge branch 'master' into xray-reconstruction

7095656

robertnishihara merged commit d49b4be into ray-project:master Aug 9, 2018

robertnishihara deleted the xray-reconstruction branch August 9, 2018 14:24

[xray] Basic task reconstruction mechanism #2526

[xray] Basic task reconstruction mechanism #2526

Conversation

stephanie-wang commented Jul 31, 2018 • edited Loading

What do these changes do?

AmplabJenkins commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jul 31, 2018

AmplabJenkins commented Jul 31, 2018

AmplabJenkins commented Jul 31, 2018

AmplabJenkins commented Jul 31, 2018

AmplabJenkins commented Aug 1, 2018

AmplabJenkins commented Aug 1, 2018

AmplabJenkins commented Aug 1, 2018

AmplabJenkins commented Aug 1, 2018

AmplabJenkins commented Aug 2, 2018

robertnishihara commented Aug 2, 2018

AmplabJenkins commented Aug 2, 2018

robertnishihara commented Aug 2, 2018

AmplabJenkins commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 6, 2018

AmplabJenkins commented Aug 8, 2018

AmplabJenkins commented Aug 8, 2018

jovany-wang Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 9, 2018

AmplabJenkins commented Aug 9, 2018

stephanie-wang commented Jul 31, 2018 •

edited

Loading

jovany-wang Aug 9, 2018 •

edited

Loading