Skip to content
This repository has been archived by the owner on Feb 20, 2024. It is now read-only.

Require the need to install kafka to different namespaces #168

Merged

Conversation

galvo
Copy link
Contributor

@galvo galvo commented Nov 5, 2018

Require the need to install kafka to different namespaces and thus the kafka advertised listeners must support dynamic firstListenerPort, currently this is hardcoded to 31090.

…e kafka advertised listeners must support dynamic firstListenerPort, currently this is hardcoded to 31090.
@ghost
Copy link

ghost commented Nov 5, 2018

@confluentinc It looks like @galvo just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Copy link
Contributor

@qshao-pivotal qshao-pivotal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR!

  1. Maybe a helper function is better?
  2. I think we should remove original advertisedListenersOverride related code and take this approach.

@galvo
Copy link
Contributor Author

galvo commented Nov 15, 2018

Thanks, will look to reimplement this as suggested above

@qshao-pivotal
Copy link
Contributor

Could you explain more on why is this needed for installing kafka on different namespaces?

@galvo
Copy link
Contributor Author

galvo commented Dec 5, 2018

When you enable NodePort the NopePort IP range by default starts @ 31090 (firstListenerPort default value) and increments based on the number of brokers configured, so if you have 3 brokers NodePorts will be exposed on 31090, 31091, 31092. When you install Kafka in a different namespace you then need to change the firstListenerPort to some other free port range so you need to overwrite the firstListenerPort value to something other than 31090 (which is in use). However you then also need to overwrite the following
"advertised.listeners": |-
EXTERNAL://${HOST_IP}:$((31090 + ${KAFKA_BROKER_ID}))
to change from 31090 to the new firstListenerPort value.

My proposed change is to always set the advertised.listeners to use the value defined by the firstListenerPort so you only need to define this once in the helm overrides.

For the record this is a slightly un-kubernetes way of exposing external access, in my mind we shouldn't have to enable nodeport at the broker level but at the headless service level and that way it would be just a single port instead of a port for every broker, and also you would be able to leave it to kubernetes to allocate you a free port rather than have to explicitly define the incremental range of NodePorts. advertised.listeners only ever picks the 1st node port value anyway so it is slightly odd to even expose nodeports on the others. Thoughts on this?

@qshao-pivotal
Copy link
Contributor

My initial thought is to totally remove all nodeports part because, as you said, it won't work well when there're multiple kafka clusters to be provisioned in same k8s cluster(no matter in a different namespace or not). I don't see how headless with a port could solve it better, still have to have a nodeport for each of the broker, and you're expecting the headless service to return VM's hostname/IP+port instead of PodIP+Port in order to make sure the traffic is routable but headless service does not work in that way.
Three are some other options to expose Kafka on K8s to outside as I see and still trying to figure out which one should we include in helm chart:

  1. Provide a Domain, however updating DNS entries could be a time-consuming process in some of the environments.
  2. Use a LB for each broker, then we have to wait for the externalIP to be available and update Kafka's advertised.listeners, and also provisioning a lot LB may not be an option due to the cost.
  3. Use a LB+ N ports for each kafka cluster, then we have same issues with NodePort, it's hard to have multiple Kafka Clusters and also there'll be problems when trying to scaling up Kafka cluster.
  4. Maybe use a Ingress Controller that supports tcp traffic, it saves cost on LB and time on DNS update, but yeah we need a tcp supported ingress controller and also I'm concerned about the performance.

@galvo
Copy link
Contributor Author

galvo commented Jan 14, 2019

4 viable options but none are simple, whats your current thoughts on how to progress this?

Ideally it would be great if defining the node port range was not required, and if this was instead left to Kubernetes to allocate free port(s).

@dkirrane
Copy link

dkirrane commented Jan 14, 2019

Any plans this could get merged

@qshao-pivotal
Copy link
Contributor

please resolve the conflicts. I'm thinking about leveraging an ingress controller that supports Kafka traffic. It's ideal for exposing Kafka to external but might not make sense in terms of throughput, performance.

@qshao-pivotal qshao-pivotal merged commit 1b4d4bb into confluentinc:master Jan 15, 2019
@galvo galvo deleted the fix/dynamic_advertised_listeners branch June 11, 2019 15:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants