Explicitly set a host when using NodePort Service

[andersbogsnes]

Previously, if the service type was a NodePort, KubeCluster would attempt to discover the IP address by listing nodes. This is a set of permissions that not all users will have, so this PR adds the ability to explicitly set a known host.

Closes #348

[andersbogsnes]

I'm unsure about how to test this implementation, as I can't see a means of getting the node IP address when using pytest-kind.

Even docker inspect gives me a blank IP Adress

[jacobtomlinson]

Thanks so much for raising this @andersbogsnes.

I think for testing it will be tricky as the Kubernetes cluster is inside a docker container and I expect it will not be possible to access the nodeport directly. I would perhaps add some check that the external_address attribute gets set correctly when the nodeport IP is provided. But I do not expect to actually be able to connect to it.

I've added some comments below because you seem to be reusing the host kwarg which is used for a different purpose.

[andersbogsnes]

Thanks for the review! I'll focus on testing some verifications that the correct values have been set.

In terms of the API - since we don't want to reuse the host arg, and we can currently only set scheduler type in the config, I see two options.

• Set scheduler-type and NodePort-host in the config
• Have both available as kwargs in the init.

Personal preference is #2, but I fully appreciate the danger of a million-param init

[jacobtomlinson]

The host arg is used for a different purpose. I see no harm in adding an additional kwarg for this. We should also support this in the config.

[andersbogsnes]

I've implemented the two keywords nodeport_host and scheduler_service_type. I've modified the test to simulate calling KubeCluster._start() as closely as possible, as actually calling it will cause the test to fail, since it can't connect to the Kind cluster with a NodePort

[jacobtomlinson]

Hey @andersbogsnes. This PR is still marked as a draft. Is it ready for a full review yet?

[andersbogsnes]

@jacobtomlinson Thanks for the ping - had left this one a bit fallow! I believe so, incorporated your previous suggestions, though could never get through the CI tests - they would hang and time out, despite working fine locally

[jacobtomlinson]

Thanks @andersbogsnes. We recently resolved some CI issues so I'll pull in the latest changes here and see if we can get CI passing.

[jacobtomlinson]

Looks like a valid failure in CI now related to this PR. @andersbogsnes would you mind taking a look?

[andersbogsnes]

Progress 😀

I'll have a look

[jacobtomlinson]

Sorry we never got to merging this. The project has changed a lot since this was raised so going to close it out.