Explicitly set a host when using NodePort Service #352
Conversation
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.
|
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 |
There was a problem hiding this comment.
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.
|
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.
Personal preference is #2, but I fully appreciate the danger of a million-param init |
|
The |
Added the option to set scheduler_service_type in the KubeCluster init Added the nodeport_host option to KubeCluster which allows the enduser to override the default behaviour of listing all nodes to get the IP of a node.
Simulates running _start, as kind doesn't support NodePorts
|
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 |
Co-authored-by: Jacob Tomlinson <[email protected]>
Co-authored-by: Jacob Tomlinson <[email protected]>
Co-authored-by: Jacob Tomlinson <[email protected]>
|
Hey @andersbogsnes. This PR is still marked as a draft. Is it ready for a full review yet? |
|
@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 |
|
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. |
|
Looks like a valid failure in CI now related to this PR. @andersbogsnes would you mind taking a look? |
|
Progress 😀 I'll have a look |
|
Sorry we never got to merging this. The project has changed a lot since this was raised so going to close it out. |
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