Allow the use of user-provided provisioner (resolves #2806).

[saimeCS]

No description provided.

[DailyDreaming]

I agree with the spirit of this but I'm wondering if adding the provisioner you want to use directly wouldn't be easier? That way unittests could help ensure it's maintained and other people might get the benefit of it.

[adamnovak]

So the idea here is that if you pass the name of a class in an installed module to use as a provisioner, it will use that?

I think that makes some sense, but if we're going to add a plugin system here I think we need a bit more documentation than just what's in the option explanation. There ought to be a new documentation page that explains the pluggable provisioner system, describes what the requirements for a provisioner are (i.e. it has to extend AbstractProvisioner, it has to call the base class constructor with super, it has to live as a top-level class in the module it is in, etc.). There should also be a minimal working example provisioner plugin (I suppose with a setup.py and directory structure to install it as the right module name?) that people can use as a base.

I'm also not sure about the "Invalid provisioner" message; since it's going to show up when the requested provisioner string is a perfectly fine specification but the provisioner it mentions is not installed/missing a dependency/has a typo in its source code, it probably should be more like "Unable to load provisioner".

We might want to think about making the batch systems (and job stores?) pluggable like this as well, in which case we'd want one unified way to load these plugins by name. On the other hand, those need to be accessed from the worker as well, which means we'd have the problem of hot-deploying the plugins that would be responsible for telling us how to hot-deploy, whereas the provisioner operates only on the leader.

[adamnovak]

OK, we can add documentation later, but the problem that we really need to fix with this PR is failing unit tests. The Python 2.7 tests fail due to the lack of a unittest.mock module in that version. Can you write the test to not require it? Otherwise we can't pull this in until we drop Python 2 support in January.

The other problem is that the provisioner tests assume that all the provisioners are available, but they aren't protected with annotations or anything to ensure that they run only when the right extras are installed. For example, the Travis tests don't have azure available, so we get:

 ProvisionerTest.test_clusteFactory_should_return_AzureProvisioner_instance_when_azure 

Traceback (most recent call last):

  File "/opt/python/3.6.7/lib/python3.6/unittest/mock.py", line 1071, in _dot_lookup

    return getattr(thing, comp)

AttributeError: module 'toil.provisioners.azure' has no attribute 'azureProvisioner'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "/opt/python/3.6.7/lib/python3.6/unittest/mock.py", line 1171, in patched

    arg = patching.__enter__()

  File "/opt/python/3.6.7/lib/python3.6/unittest/mock.py", line 1227, in __enter__

    self.target = self.getter()

  File "/opt/python/3.6.7/lib/python3.6/unittest/mock.py", line 1397, in <lambda>

    getter = lambda: _importer(target)

  File "/opt/python/3.6.7/lib/python3.6/unittest/mock.py", line 1084, in _importer

    thing = _dot_lookup(thing, comp, import_path)

  File "/opt/python/3.6.7/lib/python3.6/unittest/mock.py", line 1073, in _dot_lookup

    __import__(import_path)

  File "/home/travis/build/DataBiosphere/toil/src/toil/provisioners/azure/azureProvisioner.py", line 33, in <module>

    from azure.common.credentials import ServicePrincipalCredentials

ModuleNotFoundError: No module named 'azure'

The AuzerProvisioner test probably needs a @needs_azure annotation, like here:

toil/src/toil/test/provisioners/azureProvisionerTest.py

Line 31 in 1c9d39b

@needs_azure

Similarly, the other provisioner tests that depend on optional dependencies need similar annotations.

@saimeCS Would you be able to make these modifications?

[adamnovak]

I'm going to close this out, since we never actually did the work to pull it in.

I think the project plan right now is to lean more on clusters that know how to scale themselves (via e.g. the Kubernetes-integrated autoscaler) and to try and remove Toil's integrated autoscaler, and eventually (if we can find a replacement) get Toil itself out of the business of building and tearing down clusters. So while we could do #2806 it might be better to have someone who would want to write a Toil provisioner instead write a Kubernetes cluster autoscaler backend, and then run Toil on that. OVH, who was the original target for this work, seems to support autoscaling Kubernetes clusters.

The problem there is getting a Toil job store that works well on Kubernetes outside AWS; we need #3569 to be able to use just an S3 API for storage, and we might need some extra logic in the KubernetesBatchSystem to handle auth that doesn't look exactly like the auth on real AWS.