Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set client.rack based on topology zone #3794

Open
joke opened this issue Mar 28, 2024 · 9 comments
Open

Set client.rack based on topology zone #3794

joke opened this issue Mar 28, 2024 · 9 comments
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature-request triage/accepted Issues which should be fixed (post-triage) triage/needs-discussion

Comments

@joke
Copy link

joke commented Mar 28, 2024

Problem

Kafka is rack aware. Clients should specify the client.rack. Brockers should specify broker.rack.

AWS sets broker.rack to the availability zone. Knative could set the client.rack to node label topology.kubernetes.io/zone.

Using rack awareness would reduce cross availability zone traffic costs.

Persona:

System Operator

Exit Criteria

Consumers do set the client.rack based on the topology.kubernetes.io/zone.

@joke joke changed the title Set client.rack based on availabitly zone Set client.rack based on topology zone Mar 28, 2024
Copy link
Contributor

github-actions bot commented Jul 4, 2024

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 4, 2024
@Cali0707
Copy link
Member

/remove-lifecycle stale

@pierDipi this seems like a reasonable request, can we discuss design this week on the Eventing call?

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2024
@Cali0707
Copy link
Member

It seems like Strimzi makes this configurable through a spec like:

kafka:
  rack
    topologyKey: topology.kubernetes.io/zone

IMO, a flag like kafka-rack-topology-key in the config-kafka-features configmap could make sense here. If present, we could check if there is a label on the node for that, and if there we can use the value to set client.rack

@joke
Copy link
Author

joke commented Jul 31, 2024

Maybe the kafka-rack-topology-key be set to topology.kubernetes.io/zone. After all that's the default annotation.

@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2024

We need to use something like this

      containers:
      - name: ...
        image: ...
        env:
        - name: CLIENT_RACK
          valueFrom:
            fieldRef:
              fieldPath: metadata.labels['topology.kubernetes.io/zone']

and then set in when we read the producer / consumer configurations in (Main.java for receiver and dispatcher)

We need to figure out what happen when that key doesn't exists and if it doesn't work, maybe we support injecting that env variable but we don't set it by default

@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2024

/help

Copy link

knative-prow bot commented Aug 1, 2024

@pierDipi:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow knative-prow bot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Aug 1, 2024
@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2024

/triage accepted

@knative-prow knative-prow bot added the triage/accepted Issues which should be fixed (post-triage) label Aug 1, 2024
@joke
Copy link
Author

joke commented Aug 3, 2024

We need to use something like this

      containers:
      - name: ...
        image: ...
        env:
        - name: CLIENT_RACK
          valueFrom:
            fieldRef:
              fieldPath: metadata.labels['topology.kubernetes.io/zone']

and then set in when we read the producer / consumer configurations in (Main.java for receiver and dispatcher)

We need to figure out what happen when that key doesn't exists and if it doesn't work, maybe we support injecting that env variable but we don't set it by default

As far as I know the topology.kubernetes.io/zone are only available on the nodes. They are not automatically set on the pod. Thus using a fieldRef will not be working.

The nodeName on the other hand can be injected as an environment variable. That could be used during startup to query the Kubernetes API to obtain the node information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature-request triage/accepted Issues which should be fixed (post-triage) triage/needs-discussion
Projects
None yet
Development

No branches or pull requests

3 participants