[dashboard agent] Catch agent port conflict #23024

SongGuyang · 2022-03-10T20:15:42Z

Why are these changes needed?

Catch all the port conflicts in agent and stay agent alive. There are three port:
- grpc port
- metric exporter port
- http listening port
Turn on raylet_shares_fate_with_agent by default.
Remove the agent restart feature.
Refer to issue [Feature][runtime env] URI reference refactor #21695 (comment)

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

rkooo567 · 2022-03-11T12:16:59Z

Minimal install failures seesm related

SongGuyang · 2022-03-11T12:20:40Z

Yeah, not sure the difference of minimal env. I should improve or modify the test.

rkooo567 · 2022-03-11T12:18:01Z

dashboard/agent.py

+            try:
+                self.http_server = await self._configure_http_server(modules)
+            except Exception:
+                logger.exception(


Can you add TODO here to remove this in the future? (and do better port resolution)

rkooo567 · 2022-03-11T12:18:17Z

dashboard/agent.py

-                gcs_publisher=gcs_publisher,
-            )
-            logger.error(message)
+        # Agent is failed to be started many times.


This comment seems wrong?

rkooo567 · 2022-03-11T12:18:37Z

dashboard/agent.py

+        gcs_publisher = GcsPublisher(address=args.gcs_address)
+
+        traceback_str = ray._private.utils.format_error_message(traceback.format_exc())
+        message = (


Do we need it? In this case, raylet will just fail right?

Yeah, if we open fate sharing by default, we don't need this. I can remove this.

rkooo567 · 2022-03-11T12:19:26Z

dashboard/modules/reporter/reporter_agent.py

@@ -603,7 +603,8 @@ async def _perform_iteration(self, publisher):
            await asyncio.sleep(reporter_consts.REPORTER_UPDATE_INTERVAL_MS / 1000)

    async def run(self, server):
-        reporter_pb2_grpc.add_ReporterServiceServicer_to_server(self, server)
+        if server:


Any way to just not run this function instead of adding this logic? Seems a bit ugly

I didn't find better way. The run is a common interface of dashboard agent module. We can do anything else in this function, besides starting grpc server. So, if we don't run this function, we could lose some code paths or features.

rkooo567 · 2022-03-11T12:20:16Z

python/ray/tests/test_dashboard.py

@@ -121,8 +121,6 @@ def test_port_conflict(call_ray_stop_only, shutdown_only):
    with pytest.raises(ValueError, match="already occupied"):
        ray.init(dashboard_port=9999, include_dashboard=True)

-    sock.close()


Why removing this?

rkooo567 · 2022-03-11T12:23:52Z

python/ray/tests/test_runtime_env.py

@@ -330,13 +318,13 @@ def f():
    """
    Test task raises an exception.
    """
-    with pytest.raises(RuntimeEnvSetupError):
+    with pytest.raises(ray.exceptions.LocalRayletDiedError):


Can you check there's the error message that says the reason is due to agent failure?

I think I can remove this test. In this test(test_runtime_env_broken), we injected a env to make agent failed. Agent failure will lead to raylet failure. Raylet failure will lead to driver failure(raylet client failed). We can't catch any error message.

rkooo567 · 2022-03-11T12:24:05Z

python/ray/tests/test_runtime_env.py

        ray.get(f.options(runtime_env=runtime_env).remote())
    """
    Test actor task raises an exception.
    """
    a = A.options(runtime_env=runtime_env).remote()
-    with pytest.raises(ray.exceptions.RuntimeEnvSetupError):


Same as above

rkooo567 · 2022-03-11T12:24:41Z

src/ray/raylet/agent_manager.cc

+    RAY_LOG(INFO) << "HandleRegisterAgent, ip: " << agent_ip_address_
+                  << ", port: " << agent_port_ << ", pid: " << agent_pid_;
+  } else {
+    RAY_LOG(ERROR) << "The grpc port of agent is invalid (be 0), ip: "


Can you use WARNING instead? ERROR will propagate to the driver

rkooo567 · 2022-03-11T12:25:45Z

src/ray/raylet/agent_manager.cc

@@ -162,10 +142,11 @@ void AgentManager::CreateRuntimeEnv(

  if (runtime_env_agent_client_ == nullptr) {
    // If the agent cannot be restarted anymore, fail the request.
-    if (agent_restart_count_ >= RayConfig::instance().agent_max_restart_count()) {
+    if (disable_agent_client_ || agent_failed_) {


If agent_failed == true, this line should never reach? Maybe instead

RAY_CHECK(!agent_failed) if (disable_agent_client_)

Rmove the flag agent_failed_.

rkooo567 · 2022-03-11T12:26:17Z

src/ray/raylet/agent_manager.h

  /// Whether or not we intend to start the agent.  This is false if we
  /// are missing Ray Dashboard dependencies, for example.
  bool should_start_agent_ = true;
  std::string agent_ip_address_;
  DelayExecutorFn delay_executor_;
  RuntimeEnvAgentClientFactoryFn runtime_env_agent_client_factory_;
  std::shared_ptr<rpc::RuntimeEnvAgentClientInterface> runtime_env_agent_client_;
+  bool disable_agent_client_ = false;


Can you add a comment to explain the variable?

rkooo567 · 2022-03-11T12:26:56Z

Most of comments are about making sure we have the right message when things failed due to this issue

SongGuyang · 2022-03-11T12:31:27Z

Thanks! I will address the comments(including another PR) tonight and ping you guys again.

SongGuyang · 2022-03-11T19:18:12Z

@rkooo567 Please take a look again. One question is that can I skip the two tests? 32ce0e2

rkooo567 · 2022-03-12T00:26:25Z

I think we can remove test_dashboard_agent_restart as long as we have a test to check fate sharing.

For test_runtime_env_broken , can we just test it in a different node than the local?

Also, what's the exact current behavior when the local node is dead? Can you tell me how the error is propagated to the driver? (is driver just going to crash with check failure?)

Also can you add a test to verify fate sharing? I couldn't find it, but I might be missing something (if so, please give me the name of the function!)

rkooo567

ray/src/ray/raylet/agent_manager.cc

Line 223 in 2294a7e

CreateRuntimeEnv(job_id, serialized_runtime_env,

-> We should also handle this better. Maybe just fail the request instead of running the infinite retry? It should be fine since the agent will fate share with raylet

rkooo567 · 2022-03-12T00:29:59Z

python/ray/tests/test_dashboard.py

+    # except grpc port.
+    run_tasks_with_runtime_env()
+
+


I think we don't have a test to verify fate sharing?

rkooo567 · 2022-03-12T00:31:01Z

python/ray/tests/test_dashboard.py

-        }
-    ],
-    indirect=True,
-)
 def test_dashboard_agent_restart(


Let's just remove it and instead add a test to verify fate sharing

rkooo567 · 2022-03-12T00:31:26Z

src/ray/raylet/agent_manager.cc

-      runtime_env_agent_client_factory_(agent_ip_address_, agent_port_);
-  RAY_LOG(INFO) << "HandleRegisterAgent, ip: " << agent_ip_address_
-                << ", port: " << agent_port_ << ", pid: " << agent_pid_;
+  // Note: `agent_port_` should be 0 if the grpc port of agent is in conflict.


Can you add TODO here to remove this workaround?

rkooo567 · 2022-03-12T00:39:23Z

src/ray/raylet/agent_manager.cc

@@ -230,6 +208,11 @@ void AgentManager::CreateRuntimeEnv(

 void AgentManager::DeleteURIs(const std::vector<std::string> &uris,
                              DeleteURIsCallback callback) {
+  if (disable_agent_client_) {
+    RAY_LOG(ERROR) << "Failed to delete URIs because the agent client is disabled.";


Since it is the user facing error, we should refine it a bit more.

Based on https://spark.apache.org/error-message-guidelines.html

Failed to delete runtime environment URI because the ray agent couldn't be started due to the port conflict. See `dashboard_agent.log` for more details. To solve the problem, start ray with a hard-coded agent port. `ray start --dashboard-agent-grpc-port [port]` and make sure the port is not used by other processes.

Who encountered the problem?
What was the problem?
When did the problem happen?
Where did the problem happen?
Why did the problem happen?
How can the problem be solved?

SongGuyang · 2022-03-12T01:31:30Z

We already have the fate sharing test named test_raylet_and_agent_share_fate.

About the driver failure, the error message is:

::test_runtime_env_broken[dict] 2022-03-11 08:33:06,463	INFO worker.py:978 -- Connecting to existing Ray cluster at address: 172.18.0.3:59955
--
  | [2022-03-11 08:33:15,674 E 6912 13117] core_worker_process.cc:218: Failed to get the system config from raylet because it is dead. Worker will terminate. Status: GrpcUnavailable: RPC Error message: failed to connect to all addresses; RPC Error details:

I saw this in the https://buildkite.com/ray-project/ray-builders-pr/builds/26582#4eecbf4d-a236-4ab9-b350-10dfd971fb04

rkooo567 · 2022-03-12T02:10:20Z

We already have the fate sharing test named test_raylet_and_agent_share_fate.

About the driver failure, the error message is:
::test_runtime_env_broken[dict] 2022-03-11 08:33:06,463	INFO worker.py:978 -- Connecting to existing Ray cluster at address: 172.18.0.3:59955
--
  | [2022-03-11 08:33:15,674 E 6912 13117] core_worker_process.cc:218: Failed to get the system config from raylet because it is dead. Worker will terminate. Status: GrpcUnavailable: RPC Error message: failed to connect to all addresses; RPC Error details:
I saw this in the https://buildkite.com/ray-project/ray-builders-pr/builds/26582#4eecbf4d-a236-4ab9-b350-10dfd971fb04

Hmm. this one seems to be a pretty bad error message... Can you at least mention to the error message

core_worker_process.cc:218: Failed to get the system config from raylet because it is dead. Worker will terminate.

to take a look at raylet.out log file? I think we should find a better way to propagate error messages, but it seems to be difficult to achieve it in the short term.

Also do you happen to know why this only happens in minimal install, not a regular test?

rkooo567

LGTM given the remaining comment are addressed.

Btw, do you think this should be a part of 1.12? My impression is it could be a breaking change, and having enough time (1 release) seems to be not a bad choice. Do you guys have hard dependencies on this feature?

SongGuyang · 2022-03-12T02:31:29Z

Also do you happen to know why this only happens in minimal install, not a regular test?

I don't know the reason. Maybe the test is finished before driver detects the raylet failure.

SongGuyang · 2022-03-12T02:44:18Z

Btw, do you think this should be a part of 1.12? My impression is it could be a breaking change, and having enough time (1 release) seems to be not a bad choice. Do you guys have hard dependencies on this feature?

I think sharing fate, catching port conflict, and URI reference refactor can enhance the stability, instead of regression. Do we also have 1 or 2 weeks to test before 1.12 release? We have two choices:

option 1: Merge current PR and #22828 before the cut of 1.12. And we should do more test for this before 1.12 release.

option 2: Merge current PR and #22828 after the cut of 1.12. But we should merge it as soon as possible because it blocks a lot of following PRs.

I tend to option 1, but it's also ok to choose option 2. @edoakes @architkulkarni What do you think?

kfstorm · 2022-03-13T09:07:08Z

‼️ ACTION REQUIRED ‼️

We've updated our formatting configuration for C++ code. (see #22725)

This PR includes C++ code change. To prevent issues with merging your code, here's what you'll need to do:

Merge the latest changes from upstream/master branch into your branch.

git pull upstream master
git merge upstream/master

Resolve merge conflicts (if necessary).

After running these steps, you'll have the updated C++ formatting configuration.

Format changed files.

scripts/format.sh

Commit your changes.

git add --all
git commit -m "Format C++ code"

architkulkarni

Looks good to me, just added some nits about the wording in some of the error messages (I know some of them predated this PR, it would be nice to add them but no need to block the PR on this)

One more comment though, can we add in Sang's suggestion to the error messages?

To solve the problem, start ray with a hard-coded agent port. ray start --dashboard-agent-grpc-port [port] and make sure the port is not used by other processes.
`.

I think it would be really helpful for users who see the error message.

architkulkarni · 2022-03-14T22:36:30Z

src/ray/raylet/agent_manager.cc

+    RAY_LOG(WARNING) << "The grpc port of agent is invalid (be 0), ip: "
+                     << agent_ip_address_ << ", pid: " << agent_pid_
+                     << ". Disable the agent client in raylet.";


Suggested change

RAY_LOG(WARNING) << "The grpc port of agent is invalid (be 0), ip: "

<< agent_ip_address_ << ", pid: " << agent_pid_

<< ". Disable the agent client in raylet.";

RAY_LOG(WARNING) << "The gRPC port of the Ray agent is invalid (0), ip: "

<< agent_ip_address_ << ", pid: " << agent_pid_

<< ". The agent client in the raylet has been disabled.";

architkulkarni · 2022-03-14T22:38:10Z

src/ray/raylet/agent_manager.cc

+    RAY_LOG(ERROR) << "Raylet exits immediately because the ray agent has failed. "
+                      "Raylet fate shares with the agent. It can happen because the "
+                      "Ray agent is unexpectedly killed or failed. See "
+                      "`dashboard_agent.log` for the root cause.";


Suggested change

RAY_LOG(ERROR) << "Raylet exits immediately because the ray agent has failed. "

"Raylet fate shares with the agent. It can happen because the "

"Ray agent is unexpectedly killed or failed. See "

"`dashboard_agent.log` for the root cause.";

RAY_LOG(ERROR) << "The raylet exited immediately because the Ray agent failed. "

"The raylet fate shares with the agent. This can happen because the "

"Ray agent was unexpectedly killed or failed. See "

"`dashboard_agent.log` for the root cause.";

architkulkarni · 2022-03-14T22:42:12Z

src/ray/raylet/agent_manager.cc

+          << "Runtime environment " << serialized_runtime_env
+          << " cannot be created on this node because the agent client is disabled. You "
+             "see this error message maybe because the grpc port of agent came into "
+             "conflict. Please see `dashboard_agent.log` to get more details.";


Suggested change

<< "Runtime environment " << serialized_runtime_env

<< " cannot be created on this node because the agent client is disabled. You "

"see this error message maybe because the grpc port of agent came into "

"conflict. Please see `dashboard_agent.log` to get more details.";

<< "The runtime environment " << serialized_runtime_env

<< " cannot be created on this node because the agent client is disabled. You "

"might see this error message because the gRPC port of the agent came into "

"conflict. Please see `dashboard_agent.log` to get more details.";

architkulkarni · 2022-03-14T22:44:20Z

src/ray/raylet/agent_manager.cc

+        << "Failed to delete runtime env URIs because the agent client is disabled. You "
+           "see this error message maybe because the grpc port of agent came into "
+           "conflict. Please see `dashboard_agent.log` to get more details.";


Suggested change

<< "Failed to delete runtime env URIs because the agent client is disabled. You "

"see this error message maybe because the grpc port of agent came into "

"conflict. Please see `dashboard_agent.log` to get more details.";

<< "Failed to delete runtime env URIs because the agent client is disabled. You "

"might see this error message because the gRPC port of the agent came into "

"conflict. Please see `dashboard_agent.log` to get more details.";

architkulkarni · 2022-03-14T22:50:09Z

Btw, do you think this should be a part of 1.12? My impression is it could be a breaking change, and having enough time (1 release) seems to be not a bad choice. Do you guys have hard dependencies on this feature?

I think sharing fate, catching port conflict, and URI reference refactor can enhance the stability, instead of regression. Do we also have 1 or 2 weeks to test before 1.12 release? We have two choices:

option 1: Merge current PR and #22828 before the cut of 1.12. And we should do more test for this before 1.12 release.

option 2: Merge current PR and #22828 after the cut of 1.12. But we should merge it as soon as possible because it blocks a lot of following PRs.

I tend to option 1, but it's also ok to choose option 2. @edoakes @architkulkarni What do you think?

I agree that the stability will be improved by these two PRs, so I think option 1 would be better.

rkooo567 · 2022-03-14T23:32:00Z

src/ray/raylet/agent_manager.cc

-      [this,
-       job_id,
-       serialized_runtime_env,
+      [serialized_runtime_env,


Suggested change

[serialized_runtime_env,

[serialized_runtime_env=std::move(serialized_runtime_env),

Is it possible to do this?

We can't do this because serialized_runtime_env is a reference from task_spec.

rkooo567 · 2022-03-14T23:32:16Z

src/ray/raylet/agent_manager.cc

@@ -261,7 +234,7 @@ void AgentManager::DeleteURIs(const std::vector<std::string> &uris,
    request.add_uris(uri);
  }
  runtime_env_agent_client_->DeleteURIs(
-      request, [this, uris, callback](Status status, const rpc::DeleteURIsReply &reply) {
+      request, [callback](Status status, const rpc::DeleteURIsReply &reply) {


Suggested change

request, [callback](Status status, const rpc::DeleteURIsReply &reply) {

request, [callback=std::move(callback)](Status status, const rpc::DeleteURIsReply &reply) {

?

SongGuyang · 2022-03-15T04:00:11Z

Looks good to me, just added some nits about the wording in some of the error messages (I know some of them predated this PR, it would be nice to add them but no need to block the PR on this)

One more comment though, can we add in Sang's suggestion to the error messages?

To solve the problem, start ray with a hard-coded agent port. ray start --dashboard-agent-grpc-port [port] and make sure the port is not used by other processes.
`.

I think it would be really helpful for users who see the error message.

Good comments! Fixed!

SongGuyang added 2 commits March 11, 2022 03:35

catch port conflict in agent

fa51a76

remove the agent restart feature

15c286b

SongGuyang requested a review from rkooo567 March 10, 2022 20:15

SongGuyang assigned architkulkarni, edoakes and rkooo567 Mar 10, 2022

SongGuyang added 3 commits March 11, 2022 04:33

fix

d2f6229

fix cpp test

1c7ecad

Merge branch 'master' into dev_catch_agent_port_conflict

b490483

rkooo567 reviewed Mar 11, 2022

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 11, 2022

SongGuyang added 3 commits March 12, 2022 02:47

address comments

dc9126d

skip tests

32ce0e2

address comment

2d2539e

SongGuyang removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 11, 2022

rkooo567 reviewed Mar 12, 2022

View reviewed changes

rkooo567 approved these changes Mar 12, 2022

View reviewed changes

Merge branch 'master' into dev_catch_agent_port_conflict

abb8c49

SongGuyang added 2 commits March 14, 2022 18:21

address comments

c3f2f53

fix

355d5b2

architkulkarni approved these changes Mar 14, 2022

View reviewed changes

rkooo567 reviewed Mar 14, 2022

View reviewed changes

SongGuyang added 2 commits March 15, 2022 10:33

Merge branch 'master' into dev_catch_agent_port_conflict

6dfcec3

address comments

33bfb72

raulchen merged commit f659717 into ray-project:master Mar 15, 2022

raulchen deleted the dev_catch_agent_port_conflict branch March 15, 2022 08:09

	[serialized_runtime_env,
	[serialized_runtime_env=std::move(serialized_runtime_env),

	request, [callback](Status status, const rpc::DeleteURIsReply &reply) {
	request, [callback=std::move(callback)](Status status, const rpc::DeleteURIsReply &reply) {

[dashboard agent] Catch agent port conflict #23024

[dashboard agent] Catch agent port conflict #23024

Conversation

SongGuyang commented Mar 10, 2022 • edited Loading

Why are these changes needed?

Checks

rkooo567 commented Mar 11, 2022

SongGuyang commented Mar 11, 2022

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

rkooo567 commented Mar 11, 2022

SongGuyang commented Mar 11, 2022

SongGuyang commented Mar 11, 2022 • edited Loading

rkooo567 commented Mar 12, 2022 • edited Loading

rkooo567 left a 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

SongGuyang commented Mar 12, 2022

rkooo567 commented Mar 12, 2022 • edited Loading

rkooo567 left a comment

Choose a reason for hiding this comment

SongGuyang commented Mar 12, 2022

SongGuyang commented Mar 12, 2022

kfstorm commented Mar 13, 2022

‼️ ACTION REQUIRED ‼️

architkulkarni left a 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

architkulkarni commented Mar 14, 2022

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

SongGuyang commented Mar 15, 2022

SongGuyang commented Mar 10, 2022 •

edited

Loading

SongGuyang commented Mar 11, 2022 •

edited

Loading

rkooo567 commented Mar 12, 2022 •

edited

Loading

rkooo567 commented Mar 12, 2022 •

edited

Loading