Concurrently execute a single drush command on multiple sites (con'd)

Hey, this looks awesome! Haven't tried the code yet, but the integrate stuff looks correct as you have it.

I feel encouraged to try to plug this in to the regular drush @sitelist some-command handler, so that it would run any time you needed it to. This would be a pretty simple matter of just gening up appropriate $backend_options for each site and calling the new function. I'd have to say that there should be an option to control output, because by default it will look something like this:

site1 >> Some output
site2 >> Different output
site1 >> Some more output
site1 >> Some even more output
site2 >> Different output continues

This is exactly what you'd want if the output was from some lengthy operation (e.g. status logs), but it would be mighty annoying for just a drush status. So by default, I'd say that when concurrency > 1, output would be held in the bucket until the command completed, and then output all at once when the pipe is closed (n.b. commands are still executing concurrently). This is for use from the cli; when called via the API, then the default should be to output everything as soon as it became available.

I note that your current implementation does not yet implement concurrency < N, but it seems this would be pretty easy. You'd want to change your main while loop to while ((sizeof($open_processes)) || (sizeof($cmd))), then move the call to proc_open inside the while loop, popping an item to be opened off of $cmd if concurrency > sizeof($open_processes).

Whats the next step here?

@msonnabaum had an updated version at BADCamp, I think, that hasn't been posted here yet.

No longer applies cleanly :(. We'd really like to get this in and hook up drush_make as per #1267228-28: Drush Make should use Drush core's native download abilities concurrently

I've started to clean up #12 per #1267228-17. Still need to do a little more work on it before it's ready to post.

Here's a snapshot just for the curious. A lot of stuff works, but there are some variables not being passed through correctly, so a lot of tests are failing too. Will keep working on this.

Here is a patch that passes all tests (except for one complete test case, which I expect is an ordering issue. :p).

Works really well, and has a lot of coverage with existing tests. In the site alias test, I had to sort the output, because when php-eval is executed multiple times concurrently, the output can appear in any order. The test's expected results look odd, but it works. :)

There is no way to disable concurrent execution right now, and backend invoke will always spawn as many processes as it can all at once. Perhaps we should put in some throttling, and throttle to 1 to disable concurrency? I don't know if it's important. Output is usually ordered sanely due to the fact that different sites take different amounts of time to respond, and output is sent in chunks. Efficiency and readability for free! We may find instances where this is not the case, but for now it seems to be working pretty well.

The other thing I didn't put in was Mark's 'concurrency' switch, that allows the same command to be executed on the same site multiple times. I'm not sure how you want to use that or put that in, but it might work to just add your array_fill to drush_invoke_process. Note that #output-label is now handled at a very low level, so something reasonable will be filled in if you omit it. Set 'no-label' to get rid of it.

Oh, note that the function drush_invoke_concurrent() was not doing anything at all except calling through to backend invoke, so I just renamed drush_backend_invoke_sitealias_command to drush_backend_invoke_concurrent, and left it at that.

drush_invoke_process still works with its previous function signature, so clients (e.g. features) won't have to rev again.

I'm quite concerned about 'now way to turn concurrency off and no way to limit it'. that sounds like a recipe for swamping servers. imagine trying top run updatedb on a site list containing 1000 sites, you would swamp the server where drush is running, the DB server, and maybe more than i can't think of.

Regarding #18, I defined this:

$aliases['two'] = array(
  'site-list' => '@gk.dev,@sos.dev',
);

Then when I run this:

drush -v @two status

It works fine. I just started with master and applied the patch from #17 clean; not sure how your test might be different.

Limits won't be hard to put in; I'll take care of #19.

Here is a patch for testing that hard-codes the process limit to two. Just add some intelligence, maybe a drush_get_option somewhere, and it'll be all set.

Still interested to find out why this isn't working for Mark.

Ah, I see the difference. Local sites are working, remote sites emit no output.

Only had a little bit of time to look at this, but I verified that the cli options are the same with and without the patch (thank you, git stash, for rapid regression capability). It seems that I have a really old drush on my remote server; old drush puts the backend output in the json-encoded return variable, and new drush emits it in real time, prior to the output of the json-encoded results. Prior to this patch, drush would emit the output from the json results if there was no output before the start-marker; just need to put this logic back in.

As a workaround, try upgrading drush to HEAD on your remote machine and see if it starts working.

Here is an update that works better with old versions of Drush. Output from the json 'output' field will be displayed if Drush does not emit any real-time output before the json start-marker. Note that there is a limitation in that --debug mode with old versions of remote Drush interfere with the backwards compatibility feature, as debug messages printed out by old Drush are interpreted as regular incremental output by new Drush. It would be possible to improve this, but I don't know how much effort we want to spend on backwards compatibility.

I also altered the return value of drush_invoke_process. Previously (in #10), if there was one process, the return value would be the same as it had been in previous versions of Drush (a $values array with 'output', etc.), but if there were two processes, all of the $value arrays would be merged together with an array + operation. This causes significant data loss, so I changed it to return an array with an element 'multiple' that contains an associative array of $site => $value mappings. If clients call drush_invoke_process with a single site, then they will get a compatible result returned, but they will need to know to test for 'multiple' in the result if a multi-site operation was desired.

If there was a reasonable way to merge together the results, then I'd say maybe we need a flag in $backend_options that causes the 'multiple' return value. I'm not sure if there's anything we can do to make code written to work with a single site happy with the return values from multiple concurrent processes. Thoughts on this?

Wrong patch in #25; here's the right one.

I think we could get more test coverage here, no? If system level tests are hard to think of, test a few key functions with unit tests.

Mark or Greg is welcome to commit this when you think it is ready. I'm not able to provide more helpful feedback.

I'll add tests as time permits. I think this is ready to commit, but will wait for Mark's feedback.

I committed f41d4ba to make the complete test more stable; some code changes (such as this patch) changed the order of the completion candidates, causing spurious test failures. That shouldn't happen any more.

Are you still using an old version of drush on the remote end? Make sure that you're up to date on both sides of the communication.

Commands like status, that take a (relatively) long time to bootstrap and a comparatively short time to output will have their output grouped together, which is good. If you have long-running commands that output at arbitrary intervals, you should see the output come out as it is printed on the remote end -- unless you are running an old version of drush on the remote machine, in which case you are stuck waiting until the command is done before you will receive any output (except for log messages).

I suppose a unit test that runs concurrent commands that sleep and print output in a short loop would be useful and illustrative here.

Wait, in #29, did you mean the output returned in the $values array? In #30 I was referring to the output tha t appeared in the terminal window. We could provide terminal-window-ordered output in $values, if desired. I think that $values should also continue to have the output of each command separated out into its individual 'multiple' => '@sitename' bucket, though.

Another option for a unit test might be to finish the make integration :) Then the make tests act as an implicit test that concurrency isn't totally broken.

Per #1267228: Drush Make should use Drush core's native download abilities concurrently, we should probably hook up the 'concurrency' feature before committing this.

Here's an implementation of 'concurrency' (limits number of processes) and 'invoke-multiple' (for concurrent batch processes, like your old 'concurrency' option).

Go ahead and set the status back if you're still not satisfied with output handling.

Drush 4.5, and very old versions of Drush-5, do not support incremental output. They horde all of their output until everything is done, and then emit it all at once. You need to use a recent version of Drush-5 to get incremental output. Furthermore, if you are using Drush 4.5, --debug breaks backwards compatibility, and hides the stdout output. There is nothing we can do to get incremental output out of Drush-4, but if we worked harder on Drush-5, we could fix the problem with --debug.

Committed. Test coverage is actually already pretty good, but we could use some tests for 'concurency', etc.