Follow-up on removing report and safe from Worker.close #6423

gjoseph92 · 2022-05-23T17:06:47Z

Tests added / passed
Passes pre-commit run --all-files

Would have caught the regression in https://github.com/dask/distributed/pull/6363/files/1e260fae8c9c643754d0ddad6a791b98c50de211#r878480001

This isn't necessary, we could leave it. But I find `terminate` vs `close` confusingly-named, and it's not really going to be used now. I think this might be better added as part of dask#6387? Could go either way.

distributed/tests/test_nanny.py

gjoseph92 · 2022-05-23T17:08:08Z

distributed/scheduler.py

+        else:
+            self.running.discard(ws)


Resolves https://github.com/dask/distributed/pull/6363/files#r878475185

This is equivalent to main before the PR:

distributed/distributed/scheduler.py

Lines 4747 to 4777 in 41a54ee

def handle_worker_status_change(

self, status: str, worker: str, stimulus_id: str

) -> None:

ws = self.workers.get(worker)

if not ws:

return

prev_status = ws.status

ws.status = Status.lookup[status] # type: ignore

if ws.status == prev_status:

return

self.log_event(

ws.address,

{

"action": "worker-status-change",

"prev-status": prev_status.name,

"status": status,

},

)

if ws.status == Status.running:

self.running.add(ws)

recs = self.bulk_schedule_after_adding_worker(ws)

if recs:

client_msgs: dict = {}

worker_msgs: dict = {}

self._transitions(recs, client_msgs, worker_msgs, stimulus_id)

self.send_all(client_msgs, worker_msgs)

else:

self.running.discard(ws)

I think this is required for some tests. https://github.com/dask/distributed/pull/6363/files#r879719934

I agree than an explicit test would be better but I expect something to break. If CI is happy, I'm happy, though.

gjoseph92 · 2022-05-23T17:08:36Z

distributed/tests/test_nanny.py

+
+
+@gen_cluster(Worker=Nanny, nthreads=[("", 1)], timeout=10)
+async def test_scheduler_crash_doesnt_restart(s, a):


This fails on main

gjoseph92 · 2022-05-23T17:10:19Z

distributed/worker.py

-    async def terminate(self, **kwargs):
-        return await self.close(nanny=True, **kwargs)
-


I opted to remove Worker.terminate since it won't be used now that the Worker.close default is to also close the Nanny. I think we will want two different methods eventually, but found the naming of terminate vs close very confusing—it should be something like terminate vs maybe_restart?

If others would prefer to see terminate stick around, I'm happy to remove this commit and leave it.

gjoseph92 · 2022-05-23T17:11:43Z

distributed/worker.py

    @log_errors
    async def close(
        self,
        timeout=30,
        executor_wait=True,
-        nanny=False,
+        nanny=True,


By changing the default back to True, resolves:

https://github.com/dask/distributed/pull/6363/files#r878487787

https://github.com/dask/distributed/pull/6363/files#r878491269

https://github.com/dask/distributed/pull/6363/files#r878480001

https://github.com/dask/distributed/pull/6363/files#r878494201

This reverts commit ae9a98f.

fjetter · 2022-05-23T17:46:32Z

I'm 100% fine with reverting the default for close(nanny=True). I toggled this because I think the worker should not even have the ability to close the nanny and wanted to introduce small, incremental changes to this. That was premature and we should indeed revert it.

Regarding the remove_worker on status change, I believe we require this. If this is blocking the PR I would propose to not revert this to the original code and keep my version. We may want to add a couple of tests but I think we should merge the nanny toggle reversion regardless and the tests should be a follow up PR (i.e. not necessarily release blocking)

pentschev · 2022-05-23T18:33:38Z

Just FYI, this doesn't resolve #6320 , both Dask-CUDA tests hang and the equivalent tests that were added in #6363 distributed/cli/tests/test_dask_worker.py::test_error_during_startup and distributed/deploy/tests/test_local.py::test_localcluster_start_exception are also failing in CI.

github-actions · 2022-05-23T18:47:43Z

Unit Test Results

      15 files +      15       15 suites +15 6h 26m 7s ⏱️ + 6h 26m 7s
  2 809 tests +  2 809   2 729 ✔️ +  2 729   79 💤 +  79 1 ❌ +1
20 826 runs +20 826 19 896 ✔️ +19 896 929 💤 +929 1 ❌ +1

For more details on these failures, see this check.

Results for commit e3ab594. ± Comparison against base commit d84485b.

♻️ This comment has been updated with latest results.

This reverts commit d6e513c.

This was breaking because: * Worker failed to start * `Server.start` then calls `self.close()` * `Worker.close` was calling the `close_gracefully` RPC on the Nanny * For some reason, the RPC on the Nanny was unresponsive. I can't find an explanation for this. The event loop was not blocked. The listener seemed to be running. This definitely needs more investigation but I didn't want to look into it right now. * The worker was waiting forever on the nanny to respond, the nanny was waiting forever on the worker to close.

jrbourbeau · 2022-05-23T21:23:44Z

the equivalent tests that were added in #6363 distributed/cli/tests/test_dask_worker.py::test_error_during_startup and distributed/deploy/tests/test_local.py::test_localcluster_start_exception are also failing in CI.

Can confirm these now pass for me locally with Gabe's latest updates. @pentschev do those Dask-CUDA tests also pass?

quasiben · 2022-05-23T21:32:10Z

Can confirm these now pass for me locally with Gabe's latest updates. @pentschev do those Dask-CUDA tests also pass?

I'm still seeing this fail/hang on test_localcluster_start_exception

py.test -s -v distributed/deploy/tests/test_local.py::test_localcluster_start_exception --runslow

quasiben · 2022-05-23T21:34:07Z

Oops, forgot to pull latest changes. This passes as does my testing of Dask-CUDA

quasiben · 2022-05-23T22:04:21Z

@pentschev is asleep but also confirmed that this fixes the immediate issues in dask-cuda

Co-authored-by: Florian Jetter <[email protected]>

gjoseph92 · 2022-05-23T22:16:34Z

Seems like this is good to go? Do the failures seem flaky to others?

#6426 is a follow-up issue, but not a blocker.

jrbourbeau · 2022-05-24T00:55:07Z

Thanks @gjoseph92! Given the current time, let's give @fjetter a chance to take a look and then once this PR is in I'll push out the release (hopefully early morning CT)

fjetter · 2022-05-24T09:17:33Z

distributed/worker.py

+        if self.status in (Status.closed, Status.closing, Status.failed):
            await self.finished()
            return

+        if self.status == Status.init:
+            # If the worker is still in startup/init and is started by a nanny,
+            # this means the nanny itself is not up, yet. If the Nanny isn't up,
+            # yet, it's server will not accept any incoming RPC requests and
+            # will block until the startup is finished.
+            # Therefore, this worker trying to communicate with the Nanny during
+            # startup is not possible and we cannot close it.
+            # In this case, the Nanny will automatically close after inspecting
+            # the worker status
+            nanny = False


I added another commit to be more explicit about the stati and added a more thorough explanation of why this is happening

cc @gjoseph92

fjetter · 2022-05-24T10:56:09Z

I'm not sure if the test failure #6430 is related

jrbourbeau · 2022-05-24T14:18:34Z

Thanks all!

gjoseph92 added 7 commits May 23, 2022 11:05

Don't prematurely remove closing workers

ae9a98f

Change Worker.close default back to close Nanny

b653933

Add test_scheduler_crash_doesnt_restart

e9346cc

Would have caught the regression in https://github.com/dask/distributed/pull/6363/files/1e260fae8c9c643754d0ddad6a791b98c50de211#r878480001

clarify nanny message comment

ccc356a

note remove_worker should be used

1aa55d0

Note close-stream is unnecessary

7370386

Remove Worker.terminate

0d932b4

This isn't necessary, we could leave it. But I find `terminate` vs `close` confusingly-named, and it's not really going to be used now. I think this might be better added as part of dask#6387? Could go either way.

fjetter reviewed May 23, 2022

View reviewed changes

distributed/tests/test_nanny.py Outdated Show resolved Hide resolved

gjoseph92 commented May 23, 2022

View reviewed changes

Revert "Don't prematurely remove closing workers"

d6e513c

This reverts commit ae9a98f.

quasiben mentioned this pull request May 23, 2022

Update ImportError tests with --pre-import rapidsai/dask-cuda#914

Merged

jrbourbeau mentioned this pull request May 23, 2022

Release 2022.05.1 dask/community#245

Closed

gjoseph92 added 2 commits May 23, 2022 14:58

Revert "Revert "Don't prematurely remove closing workers""

a921eed

This reverts commit d6e513c.

Update distributed/tests/test_nanny.py

ba74495

Co-authored-by: Florian Jetter <[email protected]>

jrbourbeau requested a review from fjetter May 24, 2022 00:53

jakirkham requested a review from pentschev May 24, 2022 06:09

Do not attempt to connect to Nanny if in status==init

e3ab594

fjetter reviewed May 24, 2022

View reviewed changes

fjetter approved these changes May 24, 2022

View reviewed changes

fjetter mentioned this pull request May 24, 2022

test_ucx_unreachable failed on gpuCI #6429

Closed

fjetter mentioned this pull request May 24, 2022

distributed/cli/tests/test_dask_worker.py::test_worker_cli_nprocs_renamed_to_nworkers failure #6430

Open

jrbourbeau changed the title ~~Fix #6363 issues~~ Follow-up on removing report and safe from Worker.close May 24, 2022

jrbourbeau merged commit 30a872f into dask:main May 24, 2022

gjoseph92 deleted the fix-6363 branch May 24, 2022 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow-up on removing report and safe from Worker.close #6423

Follow-up on removing report and safe from Worker.close #6423

gjoseph92 commented May 23, 2022 •

edited by fjetter

Loading

gjoseph92 May 23, 2022

fjetter May 23, 2022

gjoseph92 May 23, 2022

gjoseph92 May 23, 2022

gjoseph92 May 23, 2022

fjetter commented May 23, 2022

pentschev commented May 23, 2022 •

edited

Loading

github-actions bot commented May 23, 2022 •

edited

Loading

jrbourbeau commented May 23, 2022

quasiben commented May 23, 2022

quasiben commented May 23, 2022

quasiben commented May 23, 2022

gjoseph92 commented May 23, 2022

jrbourbeau commented May 24, 2022

fjetter May 24, 2022

fjetter commented May 24, 2022

jrbourbeau commented May 24, 2022

	def handle_worker_status_change(
	self, status: str, worker: str, stimulus_id: str
	) -> None:
	ws = self.workers.get(worker)
	if not ws:
	return
	prev_status = ws.status
	ws.status = Status.lookup[status] # type: ignore
	if ws.status == prev_status:
	return

	self.log_event(
	ws.address,
	{
	"action": "worker-status-change",
	"prev-status": prev_status.name,
	"status": status,
	},
	)

	if ws.status == Status.running:
	self.running.add(ws)
	recs = self.bulk_schedule_after_adding_worker(ws)
	if recs:
	client_msgs: dict = {}
	worker_msgs: dict = {}
	self._transitions(recs, client_msgs, worker_msgs, stimulus_id)
	self.send_all(client_msgs, worker_msgs)

	else:
	self.running.discard(ws)



		@gen_cluster(Worker=Nanny, nthreads=[("", 1)], timeout=10)
		async def test_scheduler_crash_doesnt_restart(s, a):

		async def terminate(self, **kwargs):
		return await self.close(nanny=True, **kwargs)

Follow-up on removing report and safe from Worker.close #6423

Follow-up on removing report and safe from Worker.close #6423

Conversation

gjoseph92 commented May 23, 2022 • edited by fjetter Loading

gjoseph92 May 23, 2022

Choose a reason for hiding this comment

fjetter May 23, 2022

Choose a reason for hiding this comment

gjoseph92 May 23, 2022

Choose a reason for hiding this comment

gjoseph92 May 23, 2022

Choose a reason for hiding this comment

gjoseph92 May 23, 2022

Choose a reason for hiding this comment

fjetter commented May 23, 2022

pentschev commented May 23, 2022 • edited Loading

github-actions bot commented May 23, 2022 • edited Loading

Unit Test Results

jrbourbeau commented May 23, 2022

quasiben commented May 23, 2022

quasiben commented May 23, 2022

quasiben commented May 23, 2022

gjoseph92 commented May 23, 2022

jrbourbeau commented May 24, 2022

fjetter May 24, 2022

Choose a reason for hiding this comment

fjetter commented May 24, 2022

jrbourbeau commented May 24, 2022

gjoseph92 commented May 23, 2022 •

edited by fjetter

Loading

pentschev commented May 23, 2022 •

edited

Loading

github-actions bot commented May 23, 2022 •

edited

Loading