Suppress errors when worker or driver intentionally disconnects. #2935

richardliaw · 2018-09-24T20:18:56Z

What do these changes do?

TODO:

Have a separate path for explicit disconnects - such as ray_terminate and out of scope.
Make sure proper handling of intentionaldisconnect
Tests

Related issue number

Addresses #2481

AmplabJenkins · 2018-09-24T21:35:15Z

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

AmplabJenkins · 2018-09-25T23:39:29Z

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

richardliaw · 2018-09-25T23:56:45Z

OK some progress has been made here - but now the following error message doesn't show when unintentionally killing worker.

A worker died or was killed while executing task 00000000fbf609bc5b5a9d2fb31bf946fac35a66.

richardliaw · 2018-09-25T23:57:03Z


In [2]: the_pid
Out[2]: 4845

In [3]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:06.338121 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [4]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:12.219475 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [5]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:12.929061 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [6]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:13.493301 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [7]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:14.239035 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [8]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:14.954322 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [9]: actor.__ray_terminate__.remote()
Out[9]: ObjectID(010000003eaa034aa23f3c4e138bf78c46d2d061)

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:53:25.543941 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [10]: actor = Counter.remote()

In [11]: ray.get(actor.pid.remote())
Out[11]: 4851

In [12]: I0925 16:53:57.202100 2316759936 client_connection.cc:133] Error handling happening...6
I0925 16:53:57.205284 2316759936 node_manager.cc:649] ProcessDisconnectClientMessage Started!
In [12]: actor = Counter.remote()

In [13]: ray.get(actor.pid.remote())
Out[13]: 4843

In [14]: actor = Counter.remote()

WARNING: Logging before InitGoogleLogging() is written to STDERR
I0925 16:54:11.261371 2316759936 local_scheduler_client.cc:59] Intentionally disconnecting client 7
In [15]: exit

richardliaw · 2018-09-26T02:12:58Z

src/common/io.cc

  /* Handle the case in which the socket is closed. */
+  // RAY_LOG(INFO) << "Socket Disconnected in read_message "
+  //               << static_cast<int64_t>(MessageType::DisconnectClient);
+  RAY_LOG(INFO) << "Socket Disconnected in read_message "


this codepath isnt hit

Right, this is a legacy Ray thing which is replaced by client_connection.cc I believe

richardliaw · 2018-09-26T02:25:54Z

src/local_scheduler/local_scheduler_client.cc

+                << static_cast<int64_t>(MessageType::IntentionalDisconnectClient);
+  // RAY_LOG(INFO) << "[LocalSchedClient] Disconnecting Client "
+  //               << static_cast<int64_t>(CommonMessageType::DISCONNECT_CLIENT);
+  write_message(conn->conn, static_cast<int64_t>(MessageType::IntentionalDisconnectClient),


for some reason, introducing this redirection causes A worker died or was killed while executing task 000000005ea57cce1217c0fddb6bd1fb3fb879f6. to no longer show, even when non-intentionally disconnecting?

Well there this is handled by a different event handler, so the behavior will be different.

Updated above comment; but actually, this is fine.

I was trying to kill -9 an idle actor and obtain the task killed stderr response, but nothing was occurring. kill -9 on a actor running a task will be fine. Shouldn't raise warnings at equal levels?

AmplabJenkins · 2018-09-26T02:59:31Z

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

AmplabJenkins · 2018-09-26T03:50:45Z

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

AmplabJenkins · 2018-09-26T03:54:56Z

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

src/ray/raylet/node_manager.cc

    const std::shared_ptr<LocalClientConnection> &client) {
+  RAY_LOG(INFO) << "ProcessDisconnectClientMessage Started!";
  // Remove the dead worker from the pool and stop listening for messages.
  const std::shared_ptr<Worker> worker = worker_pool_.GetRegisteredWorker(client);


AmplabJenkins · 2018-09-26T22:33: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/8378/
Test FAILed.

robertnishihara · 2018-09-30T05:12:52Z

src/local_scheduler/format/local_scheduler.fbs

  // This is sent from the local scheduler to a worker or driver.
  RegisterClientReply,
-  // Notify the local scheduler that this client is disconnecting gracefully.
+  // Notify the local scheduler that this client is disconnecting unexpectedly.


is disconnecting -> has disconnected

src/local_scheduler/local_scheduler_client.cc

+                << static_cast<int64_t>(MessageType::IntentionalDisconnectClient);
+  // RAY_LOG(INFO) << "[LocalSchedClient] Disconnecting Client "
+  //               << static_cast<int64_t>(CommonMessageType::DISCONNECT_CLIENT);
+  write_message(conn->conn, static_cast<int64_t>(MessageType::IntentionalDisconnectClient),


richardliaw · 2018-09-30T09:50:18Z

src/local_scheduler/local_scheduler_client.cc

-  write_message(conn->conn, static_cast<int64_t>(MessageType::DisconnectClient),
-                fbb.GetSize(), fbb.GetBufferPointer(), &conn->write_mutex);
+  if(conn->use_raylet) {
+    write_message(conn->conn, static_cast<int64_t>(MessageType::IntentionalDisconnectClient),


(Moving conversation here): I was trying to kill -9 an idle actor and obtain the task killed stderr response, but nothing was occurring. kill -9 on a actor running a task will be fine. Shouldn't raise warnings at equal levels?

AmplabJenkins · 2018-09-30T11:08:21Z

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

AmplabJenkins · 2018-09-30T11:14:47Z

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

AmplabJenkins · 2018-10-01T19:35:24Z

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

robertnishihara · 2018-10-01T20:25:09Z

src/ray/raylet/node_manager.cc

  } break;
-  case protocol::MessageType::DisconnectClient: {
+  case protocol::MessageType::DisconnectClient:
+  case protocol::MessageType::IntentionalDisconnectClient: {


this line is an accident, right?

Do we want to continue processing messages even when intentionally disconnecting?

Oh my mistake, I thought case protocol::MessageType::IntentionalDisconnectClient: appeared twice, but it actually appears once in ProcessClientMessage and once in ProcessNodeManagerMessage.

The "intentional disconnect" message should never be received from another node manager (only from workers/drivers), so this case here should not exist.

robertnishihara · 2018-10-01T20:56:22Z

Nice! This looks pretty good.

This PR interacts with #2997. I'd suggest merging #2997 first since it adds some relevant tests and also changes the semantics of ProcessDisconnectClientMessage a little.

AmplabJenkins · 2018-10-02T00:32:49Z

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

robertnishihara · 2018-10-02T18:41:11Z

src/ray/raylet/node_manager.cc

    // all the messages are ignored except DisconnectClient.
-    if (static_cast<protocol::MessageType>(message_type) !=
-        protocol::MessageType::DisconnectClient) {
+    if ((static_cast<protocol::MessageType>(message_type) !=


minor, but static_cast<protocol::MessageType>(message_type) comes up in 3 places in this function I think. Maybe we can do it once earlier on?

robertnishihara · 2018-10-02T18:43:38Z

src/ray/raylet/node_manager.cc

  } break;
+  case protocol::MessageType::IntentionalDisconnectClient: {
+    if (registered_worker) {
+      registered_worker->MarkDead();


I think this can be removed. Do we need it?

MarkDead I think is only used when a driver dies to essentially ignore messages from workers that were executing tasks for that driver.

However, by returning in this method and not calling client->ProcessMessages();`, I think that will happen automatically.

Well, the point of this is to avoid the warning message here:

ray/src/ray/raylet/node_manager.cc

Lines 678 to 685 in 9c14050

// TODO(rkn): Define this constant somewhere else.

std::string type = "worker_died";

std::ostringstream error_message;

error_message << "A worker died or was killed while executing task " << task_id

<< ".";

RAY_CHECK_OK(gcs_client_->error_table().PushErrorToDriver(

job_id, type, error_message.str(), current_time_ms()));

that is raised in this code path (but avoided if we set it as dead):

ray/src/ray/raylet/node_manager.cc

Lines 662 to 664 in 9c14050

const TaskID &task_id = worker->GetAssignedTaskId();

if (!task_id.is_nil() && !worker->IsDead()) {

// If the worker was killed intentionally, e.g., when the driver that created

Is there a better way?

Oh hm, can we just add a bool push_warning argument to ProcessDisconnectClientMessage?

robertnishihara · 2018-10-02T18:44:42Z

@richardliaw One thing you mentioned is that we should also push an error when a worker/actor dies that wasn't executing a task. Do you want to do that in this PR or a subsequent one?

AmplabJenkins · 2018-10-02T20:19: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/8511/
Test PASSed.

AmplabJenkins · 2018-10-03T08:12:27Z

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

AmplabJenkins · 2018-10-04T00:25:50Z

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

AmplabJenkins · 2018-10-04T05:44:11Z

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

raulchen · 2018-10-17T06:03:52Z

Hi @richardliaw @robertnishihara. I'd like to use this IntentionalDisconnect message to decide whether an actor should be reconstructed.

But I'm still confused about how this works. In raylet mode, it seems that the worker always send IntentionalDisconnect to backend. When will the worker send Disconnect? and where's the code?

Thanks.

robertnishihara · 2018-10-18T07:30:01Z

@raulchen IntentionalDisconnect is only sent when you manually call local_scheduler_client.disconnect() from Python, which only happens in the __ray_terminate__ method in actors.

Disconnect happens every time a read fails on a socket to a client. This is never sent manually, it is just done under the hood. It appears in several places in https://github.com/ray-project/ray/blob/master/src/ray/common/client_connection.cc, e.g.,

ray/src/ray/common/client_connection.cc

Lines 151 to 154 in b82fd15

    
           void ClientConnection<T>::ProcessMessage(const boost::system::error_code &error) { 
        
             if (error) { 
        
               read_type_ = static_cast<int64_t>(protocol::MessageType::DisconnectClient); 
        
             }

raulchen · 2018-10-20T02:43:52Z

@robertnishihara thanks!

richardliaw added 2 commits September 24, 2018 12:21

small log

945bfff

fix, test, comma

04d866d

fix and tests

83f25e2

richardliaw added 2 commits September 25, 2018 16:57

fbs

c7c2b21

node manager

d37393b

richardliaw commented Sep 26, 2018

View reviewed changes

richardliaw added 2 commits September 25, 2018 19:19

fixups for similar endpoints

26582fb

better parans

8eb0668

richardliaw commented Sep 26, 2018

View reviewed changes

more debugs

753f4c2

robertnishihara reviewed Sep 30, 2018

View reviewed changes

richardliaw added 3 commits September 30, 2018 02:34

comments

8215661

remove debugs

0d7dfc9

remove extra comment

53b9d7b

richardliaw changed the title ~~[wip] Separate intentional disconnects in backend~~ Separate intentional disconnects in backend Sep 30, 2018

richardliaw commented Sep 30, 2018

View reviewed changes

nit

dfa79c8

robertnishihara reviewed Oct 1, 2018

View reviewed changes

comment and lint

9c14050

robertnishihara reviewed Oct 2, 2018

View reviewed changes

Merge branch 'master' into richardliaw-fix_extra_error

2b4cc3c

fix comments

046c645

fix compilation

a2cc22f

richardliaw assigned robertnishihara Oct 3, 2018

Update node_manager.cc

2e826e7

robertnishihara approved these changes Oct 4, 2018

View reviewed changes

Fix linting

ff92e8c

robertnishihara changed the title ~~Separate intentional disconnects in backend~~ Suppress errors when worker or driver intentionally disconnects. Oct 4, 2018

robertnishihara merged commit 01bb073 into ray-project:master Oct 4, 2018

robertnishihara deleted the fix_extra_error branch October 4, 2018 07:06

robertnishihara mentioned this pull request Oct 4, 2018

[xray] Extraneous "Worker died" error message #2481

Closed

	// TODO(rkn): Define this constant somewhere else.
	std::string type = "worker_died";
	std::ostringstream error_message;
	error_message << "A worker died or was killed while executing task " << task_id
	<< ".";
	RAY_CHECK_OK(gcs_client_->error_table().PushErrorToDriver(
	job_id, type, error_message.str(), current_time_ms()));

	const TaskID &task_id = worker->GetAssignedTaskId();
	if (!task_id.is_nil() && !worker->IsDead()) {
	// If the worker was killed intentionally, e.g., when the driver that created

Uh oh!

Suppress errors when worker or driver intentionally disconnects. #2935

Suppress errors when worker or driver intentionally disconnects. #2935

Uh oh!

Conversation

richardliaw commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related issue number

Uh oh!

AmplabJenkins commented Sep 24, 2018

Uh oh!

AmplabJenkins commented Sep 25, 2018

Uh oh!

richardliaw commented Sep 25, 2018

Uh oh!

richardliaw commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardliaw Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Sep 26, 2018

Uh oh!

AmplabJenkins commented Sep 26, 2018

Uh oh!

AmplabJenkins commented Sep 26, 2018

Uh oh!

This comment was marked as resolved.

Uh oh!

AmplabJenkins commented Sep 26, 2018

Uh oh!

robertnishihara Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Sep 30, 2018

Uh oh!

AmplabJenkins commented Sep 30, 2018

Uh oh!

AmplabJenkins commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertnishihara commented Oct 1, 2018

Uh oh!

AmplabJenkins commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertnishihara commented Oct 2, 2018

Uh oh!

AmplabJenkins commented Oct 2, 2018

Uh oh!

richardliaw commented Sep 24, 2018 •

edited

Loading

richardliaw Sep 26, 2018 •

edited

Loading

robertnishihara Sep 30, 2018 •

edited

Loading