Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate partially-removed-worker state on scheduler (comms open, state removed) #6390

Open
Tracked by #6384
gjoseph92 opened this issue May 20, 2022 · 3 comments
Open
Tracked by #6384

Comments

@gjoseph92
Copy link
Collaborator

Scheduler.remove_worker removes state regarding the worker (self.workers[addr], self.stream_comms[addr], etc.), but does not close the actual network connections to the worker. This is even codified in the close=False option, which supports removing the worker state, but not telling the worker to shut down or to disconnect.

Keeping the network connections open (and listening to them) is essentially a half-removed state. The scheduler no longer knows about the worker, but if the worker sends it updates over the open connection, it will respond to them (potentially invoking handlers that assume the worker state is there).

There are two things to figure out:

  • What does it mean for a worker to be "there" or "not there", from the scheduler's perspective?
    • i.e. is it only that self.workers[addr] exists? Or also self.stream_comms[addr], and other such fields? Is there a self.handle_worker coroutine running for that worker too?
    • Can there be a single point of truth for this? A single dict to check? Or method to call?
  • How can Scheduler.remove_worker ensure that:
    • after it returns, the worker is fully "not there"
    • if it yields control while it's running (via await), things are in a well-defined state (worker is either "there", or "not there", or maybe even in a "closing" state, but no half-removed state like we have currently)
    • if multiple remove_worker coroutines run concurrently, everything remains consistent
    • if multiple remove_worker coroutines run concurrently, the second one does not return until the worker is actually removed (i.e. the first coroutine has completed)

Addresses #6354

gjoseph92 added a commit to gjoseph92/distributed that referenced this issue Jun 3, 2022
`Scheduler.restart` used to remove every worker without closing it. This was bad practice (dask#6390), as well as incorrect: it certainly seemed the intent was only to remove non-Nanny workers.

Then, Nanny workers are restarted via the `restart` RPC to the Nanny, not to the worker.
@BitTheByte
Copy link

Is there any ETA for this?

@crusaderky
Copy link
Collaborator

crusaderky commented Feb 23, 2024

There's an extra layer of complexity added to this when Scheduler.retire_workers and its parameter flags come into play:

close_workers=False, retire=False

The worker sits forever in status=closing_gracefully

close_workers=True, retire=False

Calls Scheduler.close_worker(), which kindly asks the worker to shut itself down. This API makes no sense to me.

close_workers=False, retire=True

Calls Scheduler.remove_worker(close=False) as described above.
This is the default behaviour of Scheduler.retire_workers().

close_workers=True, retire=True

Shut the workers and the nannies down and remove them.
This is the default behaviour of Client.retire_workers(), and it differs from the scheduler-side API.

@crusaderky
Copy link
Collaborator

crusaderky commented Feb 23, 2024

I suspect the below is purely hypothetical, but I'll note it nonetheless.

At the moment, there is no simple API for graceful worker restart. For example, it would make sense for cleaning up a memory leak on a worker without losing the data on it. Currently you can do

client.retire_workers([addr], close_workers=False, remove=False)
client.restart_workers([addr])

but with the removal of the flags from retire_workers, it would become impossible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants