dask-kubernetes icon indicating copy to clipboard operation
dask-kubernetes copied to clipboard

Published 20 hours ago • verified publisher icon dask

Explicitly set a host when using NodePort Service

Open andersbogsnes opened this issue 2 years ago • 9 comments

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 avatar Jul 28 '21 20:07 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

andersbogsnes avatar Jul 28 '21 20:07 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

andersbogsnes avatar Jul 29 '21 13:07 andersbogsnes

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.

jacobtomlinson avatar Jul 29 '21 14:07 jacobtomlinson

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

andersbogsnes avatar Aug 11 '21 22:08 andersbogsnes

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

jacobtomlinson avatar Oct 19 '21 09:10 jacobtomlinson

@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

andersbogsnes avatar Oct 19 '21 19:10 andersbogsnes

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 avatar Oct 21 '21 08:10 jacobtomlinson

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

jacobtomlinson avatar Oct 21 '21 09:10 jacobtomlinson

Progress 😀

I'll have a look

andersbogsnes avatar Oct 21 '21 09:10 andersbogsnes

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

jacobtomlinson avatar Apr 30 '24 15:04 jacobtomlinson