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

WIP: cluster: make scheduler configurable #11546

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
9 participants
@cjihrig
Copy link
Contributor

cjihrig commented Feb 25, 2017

This is a WIP for making the cluster scheduler configurable by users. This still needs docs, tests, etc.

In the current implementation, the cluster module exports a Scheduler class. The class provides the following methods/hooks:

  • constructor() - Used to configure data structures for managing connections (handles) and cluster workers.
  • addConnection() - Called when a new connection is received. This is where the user would store the incoming connection handle to be distributed later.
  • addWorker() - Called when a new cluster worker is added. This is where the user would add the worker into the pool of workers.
  • removeWorker() - Called when a cluster worker is being removed from the pool.
  • shutdown() - Called when the scheduler is finished working. This would be used to clean up any lingering resources such as handles.
  • schedule() - This is the scheduling algorithm. The output of the algorithm should be a connection handle to distribute, and the worker to distribute it to.

I tried to minimize the exposure of the cluster's inner workings to incoming connection handles. Worker objects are already part of the public API.

Remaining questions:

  • Should we enforce that handles from incoming connections are stored in a queue (array)? This would eliminate the need for the current addConnection() hook.
  • Should we enforce that workers are stored in a "worker id to worker object" map, like they currently are? This would eliminate the need for the addWorker() and removeWorker() hooks.
  • If we can get rid of the connections and workers hooks, then the shutdown hook can probably go. Then we could drop the Scheduler class, and just provide a schedule() function that takes the available workers and handles as input, and schedules accordingly. Ideally, I'd like to go this simpler route, but want to check with some people who are actually doing this in practice to make sure it would be flexible enough. cc: @Yemanu and @redonkulus from #10880. Would this approach work for your needs?

Closes #10880

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

cluster

lib/internal/cluster/round_robin_handle.js Outdated
const { sendHelper } = require('internal/cluster/utils');
const getOwnPropertyNames = Object.getOwnPropertyNames;
const { create, getOwnPropertyNames } = Object;

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 25, 2017

Member

not really sure I'll ever get used to that one

This comment has been minimized.

Copy link
@mscdex

mscdex Feb 25, 2017

Contributor

The variable name aliasing for this style is even harder to remember/get used to.

lib/internal/cluster/round_robin_handle.js Outdated
}
shutdown() {
for (var handle; handle = this.handles.shift(); handle.close())
;

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 25, 2017

Member

possibly neater/more readable as

var handle;
while (handle = this.handles.shift()) {
  handle.close();
}

linter might complain less too?

@mscdex mscdex added cluster and removed build labels Feb 25, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 27, 2017

This class-based approach seems way overkill. Why is plugging in a custom schedule function not sufficient?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Feb 27, 2017

I agree, and I think that it should be sufficient (see "Remaining questions" in the PR description). It really just comes down to whether or not people who do this need to define their own data structures for managing the workers and handles, or if we can standardize on the current data structures.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 1, 2017

Ping @Yemanu and @redonkulus. I'd like your feedback before dropping the class based approach.

@yemanett

This comment has been minimized.

Copy link

yemanett commented Mar 2, 2017

@cjihrig, we still want to use all functionality provided by the RR scheduler, but we want to customize the schedule(callback) method. It looks like RR is no longer visible to the public API, right?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 3, 2017

@yemanett all of the functionality will still be available, but you'll be responsible for scheduling connections to the worker yourself. For RR, that's pretty simple: you grab the first worker out of an array, then put that worker in the back of the array.

My question is whether or not you need special hooks for managing the data structures for the connection queue and workers. My guess is no.

@yemanett

This comment has been minimized.

Copy link

yemanett commented Mar 3, 2017

@cjihrig sorry I missed line https://github.com/cjihrig/node-1/blob/c47fe62a1d53959a12c741731608b7efaf5856e1/lib/internal/cluster/master.js#L300 which lead me to my previous incorrect comment.

Either way should be fine, but the Class based approach feels more intuitive. In that case we need just addWorker(), removeWorker and schedule.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 5, 2017

@yemanett may I ask how you plan to use the addWorker() and removeWorker() hooks? Note that these are called when a worker is added or removed. They are not used to add and remove workers.

If I recall correctly, your original use case was to take workers offline for some amount of time. In that scenario, I would recommend that the child process sends an IPC message to the cluster master saying that it needs to go offline. The master can then take that information into account when calling schedule().

@yemanett

This comment has been minimized.

Copy link

yemanett commented Mar 5, 2017

@cjihrig Basically, we are planning to use it with our application runner module which manages the node processes including re-spawning a new worker when a child process dies. The application runner requires cluster. The runner loads the scheduler script in the master process. The scheduler takes a process out of rotation based on its algorithm by calling the removeWorker(w) method which removes w from the ‘this.free’ list and notify the worker via IPC message. The worker can then perform some tasks and notify master via PCI to put it back in rotation, in which case master calls addWorker(w) to add w into this.free list.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 6, 2017

The scheduler takes a process out of rotation based on its algorithm by calling the removeWorker(w) method which removes w from the ‘this.free’ list and notify the worker via IPC message.

That's not how removeWorker() works. It isn't used to take a worker offline. It's a function that is called once the worker is already offline. In your case, the scheduler would take the worker out of rotation based on its own algorithm, or some message received from the worker. With the worker then marked as offline, the schedule() function just wouldn't route any requests to it.

The worker can then perform some tasks and notify master via PCI to put it back in rotation, in which case master calls addWorker(w) to add w into this.free list. In your case, the worker would send an IPC message to the master, and the master would mark the worker as online again. Then schedule() would see that worker as a valid receiver of requests.

Based on this, it still seems like schedule() is all that's needed.

Also not how addWorker() works. It would be invoked when a cluster worker comes online.

@yemanett

This comment has been minimized.

Copy link

yemanett commented Mar 6, 2017

@cjihrig yes schedule is what we need, but currently it takes worker and handle, how do we get the handle?

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Mar 6, 2017

What do you need the handle for? schedule()'s job is to pick the next worker. Ideally, you don't need to look at the handle for that.

That said, schedule() should be flexible enough to support a use case like IP-based load balancing. That's difficult though because the cluster module operates on internal handle objects, not on net.Socket objects.

@yemanett

This comment has been minimized.

Copy link

yemanett commented Mar 6, 2017

@bnoordhuis

What do you need the handle for? schedule()'s job is to pick the next worker. Ideally, you don't need to look at the handle for that.
Because the above proposal says "Then we could drop the Scheduler class, and just provide a schedule() function that takes the available workers and handles as input, and schedules accordingly."

That said, schedule() should be flexible enough to support a use case like IP-based load balancing. That's difficult though because the cluster module operates on internal handle objects, not on net.Socket objects.

Our use case simple, it is just to take a worker OOR(out of rotation) so that master won't distribute new incoming requests to the worker while the worker is OOR . We still want to continue to use RR scheduler. But we just need to add a functionality to take a worker offline. The sequence diagram look something like this

offlinescheduler

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 14, 2017

That said, schedule() should be flexible enough to support a use case like IP-based load balancing. That's difficult though because the cluster module operates on internal handle objects, not on net.Socket objects.

@bnoordhuis do you have any preference between:

  1. Dropping the handles from schedule(). IP-based load balancing wouldn't really be possible.
  2. Passing the raw handles to schedule(). This would expose internals.
  3. Wrapping the handles in a net.Socket. There is probably some performance overhead.
@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Mar 14, 2017

I'd say option 3, probably with a way to opt in so people with no need for the handle don't have to pay the overhead.

@vmarchaud vmarchaud referenced this pull request Mar 19, 2017

Open

Sticky Sessions ? #389

@cjihrig cjihrig force-pushed the cjihrig:scheduler-new branch 3 times, most recently Mar 27, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 28, 2017

@bnoordhuis I've gotten rid of the scheduler class. You would just have to pass in a scheduler function now. You can request that the socket be passed in by adding a flag to the scheduler function. Before I go about writing docs and tests, what do you think?

@bnoordhuis
Copy link
Member

bnoordhuis left a comment

Looks like an alright approach to me.

socket = new net.Socket({
handle,
readable: false,
writable: false,

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Mar 28, 2017

Member

readable = writable = false. Is that intentional?

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 28, 2017

Author Contributor

I figured that the socket should only be used to determine where to pass the handle, and not actually read or written.

@cjihrig cjihrig force-pushed the cjihrig:scheduler-new branch 2 times, most recently Mar 29, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Mar 29, 2017

While I'm here, how about a little name bikeshedding: schedule or scheduler?

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 29, 2017

SchedulerThingy ;-)

sigh... long day so far. scheduler works for me.

@yemanett

This comment has been minimized.

Copy link

yemanett commented Mar 29, 2017

@cjihrig, the scheduler works nicely for me, The changes looks good to me.

Thanks,

@cjihrig cjihrig force-pushed the cjihrig:scheduler-new branch Mar 30, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 30, 2017

@bnoordhuis updated.

  • Removed the cluster.SCHED_CUSTOM constant.
  • Added stronger language to the docs about altering workers

I went with scheduler over schedule since the function is the thing that determines the schedule.

@bnoordhuis
Copy link
Member

bnoordhuis left a comment

LGTM although the wording in the documentation about not modifying the array could still be stronger.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Apr 3, 2017

@bnoordhuis do you have any specific wording in mind? I'm happy to try to scare users.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Apr 3, 2017

"The array should under no condition be mutated"?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Apr 4, 2017

OK, I'll update to that, although we do mutate the array in the round robin scheduler.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Apr 4, 2017

Yes, but we are allowed to break our own rules.

@jasnell

jasnell approved these changes Apr 4, 2017

@cjihrig cjihrig force-pushed the cjihrig:scheduler-new branch to fefd041 Apr 7, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Apr 7, 2017

Updated to the suggested wording.

CI: https://ci.nodejs.org/job/node-test-pull-request/7265/. Of course Windows wouldn't work.

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

@yemanett

This comment has been minimized.

Copy link

yemanett commented Apr 27, 2017

The following test is failing: test/windows-fanned

@yemanett

This comment has been minimized.

Copy link

yemanett commented May 4, 2017

@cjihrig - do you know why test test/windows-fanned is failing? the 'Detail' link returns status 502

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented May 5, 2017

@yemanett the CI results are only kept around temporarily. It looks like those ones are no longer available.

@yemanett

This comment has been minimized.

Copy link

yemanett commented May 8, 2017

@cjihrig - when will this be merged ?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented May 9, 2017

@yemanett It needs a rebase on the documentation, and some work to make the CI pass on Windows.

@redonkulus

This comment has been minimized.

Copy link

redonkulus commented May 9, 2017

@cjihrig are you still actively working on this PR? If so, when do you think it can be completed?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented May 10, 2017

@redonkulus still working on it, but if you wanted to debug the test on Windows, I wouldn't try to stop you.

@yemanett

This comment has been minimized.

Copy link

yemanett commented May 30, 2017

@cjihrig do you think the PR issue will be resolved and merged soon?

@Trott Trott added the stalled label Jul 27, 2017

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jul 27, 2017

Labeling stalled. Feel free to remove the label if that's wrong.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Aug 12, 2017

Someone else can take this over if they want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.