search

jaegertracing / helm-charts

Helm Charts for Jaeger backend
Apache License 2.0
265 stars 340 forks source link

[jaeger] chore: bumping Kafka chart version to ^18.0.8 #395

Closed locmai closed 2 years ago

locmai commented 2 years ago

Signed-off-by: Loc Mai locmai0201@gmail.com

What this PR does

Upgrade Kafka chart to ^18.0.8 which contains the next major version of Kafka from 2.8 to 3.2.0

Which issue this PR fixes

Checklist

Additional notes about the Kafka upgrade

I was able to upgrade the Kafka without causing any side effect for Jaeger. And the tracing spans could flow in normally after the upgrade.

Take the guidance from Kafka into account, we could also update the document to set the server.properties right, note that I didn't take these steps, a simple version bump solve the case.

  1. Add an upgrading section for major version from Kafka
  2. Add the inter.broker.protocol.version=<current-version-to-upgrade-from> (latest: inter.broker.protocol.version=2.8.1) into the Kafka config section.
  3. Apply the change
  4. After rolling out all of the Kafka pods, remove the inter.broker.protocol.version line or set it to the one we just upgraded from (inter.broker.protocol.version=3.1.0)

Testing values:

provisionDataStore:
  cassandra: false
  elasticsearch: true
  kafka: true

storage:
  type: elasticsearch

kafka:
  replicaCount: 3
  autoCreateTopicsEnable: true
  zookeeper:
    replicaCount: 3
    serviceAccount:
      create: true

I actually tested from ^14.9.1 to ^15.5.1 and it works. Being greedy and bump it to ^18.0.8 still works with no breaking change/downtime.

Test video: https://www.youtube.com/watch?v=JQYUb0LUjds

Upgrading notes

Kafka

To 15.0.0

This major release bumps Kafka major version to 3.x series. It also renames several values in this chart and adds missing features, in order to be inline with the rest of assets in the Bitnami charts repository. Some affected values are:

  • service.port, service.internalPort and service.externalPort have been regrouped under the service.ports map.
  • metrics.kafka.service.port has been regrouped under the metrics.kafka.service.ports map.>
  • metrics.jmx.service.port has been regrouped under the metrics.jmx.service.ports map.
  • updateStrategy (string) and rollingUpdatePartition are regrouped under the updateStrategy map.
  • Several parameters marked as deprecated 14.x.x are not supported anymore.

Since by default, we don't override these values - ref - we don't need to do anything for it.

From 15.0.0 -> 16.0.0 -> 18.0.0, we have the upgrades for Zookeeper Helm chart which doesn't contain any breaking change.

For example:

To 16.0.0

This major updates the Zookeeper subchart to it newest major, 9.0.0. For more information on this subchart's major, please refer to zookeeper upgrade notes.

To 18.0.0

This major updates the Zookeeper subchart to it newest major, 10.0.0. For more information on this subchart's major, please refer to zookeeper upgrade notes.

Zookeeper

Pretty much the same for the values - and since we didn't touch much for the default one, we are safe:

To 8.0.0

Affected values: allowAnonymousLogin is deprecated.

  • containerPort, tlsContainerPort, followerContainerPort and electionContainerPort have been regrouped under the containerPorts map.
  • service.port, service.tlsClientPort, service.followerPort, and service.electionPort have been regrouped under the service.ports map.
  • updateStrategy (string) and rollingUpdatePartition are regrouped under the updateStrategy map.
  • podDisruptionBudget. parameters are renamed to pdb..

To 10.0.0

This new version of the chart adds support for server-server authentication. The chart previously supported client-server authentication, to avioud confusion, the previous parameters have been renamed from auth. to auth.client..

locmai commented 2 years ago

Thanks @mehta-ankit for approving the workflow! Do you folks think we could go with this or more document/testing need to be here?

mehta-ankit commented 2 years ago

I am always concerned with these dependency chart upgrades 😅. Can we have the change log between current version to 15.x.x and then 15.x.x to 18.x.x added to the MR. Just to see what can affect users (especially breaking changes).

locmai commented 2 years ago

Yes, here is the list of changes in the major version https://github.com/bitnami/charts/tree/master/bitnami/kafka#upgrading

To sum up:

mehta-ankit commented 2 years ago

A several values renamed: Since we are using the default one. We should be good with these

are these helm chart values or Kafka arguments passed into the container ?

locmai commented 2 years ago

they are the Helm chart values

mehta-ankit commented 2 years ago

they are the Helm chart values

Maybe i am asking for too much, but if you have a list of them, adding them to this PR would help. Not necessary though.

Any changes to Kafka/Zookeeper serviceMonitor that you noticed ? we deploy them from the kafka/zookeeper helm chart and in the past they have made breaking changes to those that affected us: https://github.com/bitnami/charts/issues/7778#issuecomment-1054644064

locmai commented 2 years ago

Maybe i am asking for too much, but if you have a list of them, adding them to this PR would help. Not necessary though.

Yeah no worry. I want to make sure we do the right things as well.

I added the list in the description, basically we didn't touch those in the default values.yaml so no issue there unless you wanted to give the users a notice like "to upgrade to this version of the Helm chart, please ensure you update the following values accordingly ..."

Any changes to Kafka/Zookeeper serviceMonitor that you noticed ?

Not really. Quoted from a maintainer over there Performed several standardizations. Did you check with the latest version of the chart? -> I could do this real quick tomorrow.

locmai commented 2 years ago

@mehta-ankit I just tested the new one, the issue is fixed over this commit https://github.com/bitnami/charts/commit/a0732d72b5fa5cd21f8568948065c99ea429c617?diff=unified#diff-5c0238c0c30e5adfa6269d4ce0f41859a9649d20248167994ff85093fe7b56c5R13-R15 - my values (for Jaeger Helm chart) to enable it:

provisionDataStore:
  cassandra: false
  elasticsearch: true
  kafka: true

storage:
  type: elasticsearch
kafka:
  replicaCount: 3
  autoCreateTopicsEnable: true
  zookeeper:
    replicaCount: 3
    serviceAccount:
      create: true
    metrics:
      enabled: true
      serviceMonitor:
        enabled: true
  metrics:
    kafka:
      enabled: true
    serviceMonitor:
      enabled: true
    jmx:
      enabled: true
mehta-ankit commented 2 years ago

bitnami/charts#7778 (comment)

So they did do that for kafka chart, but its still inconsistent across different charts, which means for zookeeper it needs to be .Values.metrics.serviceMonitor.additionalLabels: https://github.com/bitnami/charts/blob/master/bitnami/zookeeper/templates/servicemonitor.yaml#L13 But that's fine, since that is the status quo right now. So no issues 👍🏽