Use Traitlets for passing parameters to Build classes

[manics]

BinderHub is slowly gaining the ability to move beyond running on Kubernetes with Docker.

This is the first step to make it possible to configure parameters specific to the underlying infrastructure (Kubernetes, Docker, Podman, Local, etc). I've tried to minimise the changes.

See:

• Support running without kubernetes, just docker #1318
• Add capabilities/privileged to build container to support running on K8s without Docker  #1512 (comment)

The build.py changes look very large, but it's mostly rearranging the existing code:

• Common parameters and methods are moved out of Build and into the Traitlets based BuildExecutor base class (suggestions for alternative names welcome- if we want backwards compatibility we have to keep the generically named Build class as the Kubernetes builder)
• K8S specific methods are moved from Build to the Traitlets based KubernetesBuildExecutor
• The original Build class is now just a wrapper class around KubernetesBuildExecutor. The Build.__init__ function signature is unchanged, so hopefully if someone's subclassed it their class should still work with binderHub.

builder.py now checks whether the BuildClass is the old existing Build or the newer BuildExecutor based classes. If it's the former the behaviour is unchanged. If it's the latter then parameters should be set using Traitlets in a config file or similar, the current BinderHub command-line args will be ignored.

For some reason switching to Traitlets surfaced a unset IOLoop.current(), so I've switched to avoiding tornado.ioloop.IOLoop.current()other than in the BinderHub initialization code. Instead the loop is passed as a parameter to theBuild*` classes. I can try and split it into a separate PR if you prefer but it'll end up conflicting here due to the refactoring.

This PR currently includes unmerged PR:

• Check for low-level errors when submitting builds #1517

[manics]

BinderHub hard-codes the cleanup command here:

binderhub/binderhub/app.py

Lines 940 to 944 in 0fa35a0

	lambda: Build.cleanup_builds(
	self.kube_client,
	self.build_namespace,
	self.build_max_age,
	)

Though since the Build class is hardcoded maybe it's fine to get rid of the cleanup_builds() method since even if it's overridden it'll never be used?

[manics]

If we switch the default to the new traitlets class (#1521) we won't need to worry about this.

[yuvipanda]

Omg so excited for this!!!!!

[minrk]

This is great!

I left some comments, mostly around the use of IOLoop.current and references to io loops.

[manics]

Should appendix be passed in from BinderHub.appendix which therefore overrides BuildExecutor.appendix, or should it be set by the BuildExecutor.appendix Traitlet only? On one hand it's only applicable to repo2docker which points to it being configurable in BuildExecutor, on the other hand changes to appendix can result in major changes to the built images which implies it's a higher-level BinderHub feature.

[minrk]

In general, I think it's best for configurables to only show up one place, and close to the place they are used - there are likely a few things on BinderHub that should be on BuildExecutor now that it's configurable. We probably shouldn't have things that are set on BinderHub, but only to be used on BuildExecutor. That organized config a bit better, I think, and eliminates the App->settings->Build pipeline. But it's a backward-compatibility issue to move config.

[manics]

I've changed it so BinderHub.appendix is ignored by BuildExecutor.

Note BinderHub.appendix isn't used directly, it's expanded as a Jinja2 template:

binderhub/binderhub/builder.py

Lines 420 to 426 in b78cac3

	appendix = self.settings["appendix"].format(
	binder_url=self.binder_launch_host + self.binder_request,
	persistent_binder_url=self.binder_launch_host
	+ self.binder_persistent_request,
	repo_url=repo_url,
	ref_url=self.ref_url,
	)

The appendix is empty by default so in the default case this has no effect. In the past appendix was set in mybinder.org-deploy, but that was subsequently removed in jupyterhub/mybinder.org-deploy#1284 , the relevant information is now passed at runtime, so this should be OK to remove in a later breaking PR.

[consideRatio]

Wieee this looks good to me! Thank you @manics for your effort into this!!!!!

[manics]

Thanks for reviewing! I've rebased this PR onto master.

[consideRatio]

@minrk wdyt, merge?

[minrk]

Wonderful!