Simple compatibility

[granders]

Adds no-teardown option and fixes bug with log collection.

Ignore the branch name - this was meant as a companion commit to some muckrake compatibility work, but this is really just a few tiny changes.

[ewencp]

Not a major issue, but is this possibly too broad? It ignores anything ending in results. That's probably unusual for a single file (which will have an extension), but it seems like this could unintentionally match directories.

Any reason the line shouldn't be results/ which would only match the results directory at the top level?

[ewencp]

Basic patch looks good (and very helpful!). But the tests fail. It's an issue with MockArgs, which is related to one of my comments about passing in the argparse output directly rather than using, e.g., kwargs.

[granders]

thanks @ewencp , uploaded changes and filed an issue for parse args. Unit tests pass again.

[ewencp]

@granders LGTM. Two final questions.

1. Does this currently rely on the fact that we still have _kill_running_processes in Service that kills all java processes, and therefore not having a separate mechanism for clean up is working ok in muckrake tests since the cleanup just happens when you run another test? I want to make sure that the story for using this feature is actually clear for any type of project.
2. Is teardown the only thing you want, i.e. would a more general form like --phases=setup,test,teardown make sense? Not sure if it's all that useful since you can just comment out the test/use an empty one, but is it ever helpful to only have the setup phase run?

[granders]

@ewencp
1 ducktape Service class does still have this, but it does not rely on it. The primary mechanism for stopping and cleaning is overriding stop_node and clean_node on Service subclasses, and these get called by the default stop() and clean() methods.

_kill_running_processes lives on as a kludgy cleanup redundancy mechanism. It probably should be removed or made more general if people start using ducktape outside of kafka and muckrake.

2 Hm... at first glance I didn't see it as useful, but here are some thoughts:
What might use case be?

• Use as a way to setup cluster from command line without running test (ducktape test.py --phases=setup) This might actually be useful. But given the blurry line between setup and constructors, it might not be as useful as we think.
• Tear stuff down without running test. This also could be useful if we had previously run a test without tearing down. However, implementation is tricky given that services are often registered in test_context within the test logic itself.
• running a test without setup doesn't make much sense, and is accomplished in any case by not overriding setup (default implementation is a noop)

[ewencp]

1. Right, this is supposed to be removed, but I thought we were possibly leaving it in place until we had a general mechanism in place that could be used on a per-project basis. Specifically for this case, I don't think stop() and clean() cover the case I was worried about. If you run a test with --no-teardown, isn't the whole point that it doesn't run stop() and clean()? Which means subsequent tests might conflict, right? What allows you to be certain without manually cleaning off the nodes?
2. I'm thinking about how you would use this for debugging/building a new test. A lot of test have setup like start ZK + Kafka, then the body of the test like run producer performance + consumer. If you're iterating on a test, separating all three steps is potentially useful as you fix errors in them, though running the body of the test is the one I'd envision getting reused the most since that's where most of the interesting logic is. You can do this by deleting/commenting the setup step, but having support built in might be easier. Anyway, it's not critical, but it wasn't clear to me why tearDown was really special wrt this. There are, of course, other issues like how the information about which cluster nodes were being used would be maintained across multiple runs.

[granders]

1. Yes, without kill_running_processes, you might get potential conflicts. Even with it, you still have potential for the issue we've talked about with stray processes on nodes that don't get used in the next test that gets run. Maybe this should be emphasized in the comment. When developing tests, I have simply rerun the test with teardown re-enabled, but that is not a great solution. Either you'd want to remember state across test runs or be able to run stop and clean on all nodes from the command line.
2. I see your point here, this could be convenient when developing tests. The trouble with no-setup is, as you said, maintaining state across multiple runs and allowing services to allocate specific already-set-up nodes from the cluster. teardown may be special only in that it is simplest to disable.

[ewencp]

Right, and I've tried tracking state across test runs before. It tends to become a huge mess since there's potentially arbitrary state per service and test, which means you need to provide a storage API to both services and tests. Plus track enough stuff about the current cluster state to restore things properly. Plus it tends not to handle external intervention well (e.g. a service was supposedly running, but you ran vagrant halt + vagrant up in between and now nothing is running).

So if we just leave it with the option as implemented, how should we handle the cleanup phase. Do we need a separate command for that? Or should we defer that to another issue which addresses the issue generally, and allows for a project-specific implementation?

[granders]

Ok, no state tracking.

The solution to cleanup phase (even a hacky one) requires a bit of planning still, so I'll defer to #56

[granders]

Added wait_until method to address the common pattern of waiting for some condition and timing out

[granders]

@ewencp This is ready for another look.

[ewencp]

LGTM