handle upgrade kafka from zookeeper to kraft #1284

ldpliu · 2024-12-17T09:07:53Z

Summary

Related issue(s)

Fixes #
https://issues.redhat.com/browse/ACM-16108

Tests

Unit/function tests have been added and incorporated into make unit-tests.
Integration tests have been added and incorporated into make integration-test.
E2E tests have been added and incorporated into make e2e-test-all.
List other manual tests you have done.

yanmxa · 2024-12-18T07:50:52Z

operator/pkg/controllers/transporter/protocol/manifests/global-hub-kafka-node-pool.yaml

@@ -1,7 +1,7 @@
 apiVersion: kafka.strimzi.io/v1beta2
 kind: KafkaNodePool
 metadata:
-  name: kafka
+  name: kraft


why change the name to Kraft? It seems wired to name a resource to kraft.

why change the name to Kraft? It seems wired to name a resource to kraft.

If we do not change the name, the pvc will conflict in upgrade env.

Kraft is an algorithm. Maybe we can just name it kafka-node-pool or kafka-cluster to distinguish it from the original one.

Kraft is an algorithm. Maybe we can just name it kafka-node-pool or kafka-cluster to distinguish it from the original one.

Sure, we could change it, @clyang82 what's your suggestion?

How about use name: "global-hub" as kafkanodepool name

There are several options for now:

global-hub

global-hub-kafka

multicluster-global-hub-kafka

kafka-cluster

kafka-node-pool

I suggest using the name multicluster-global-hub-kafka for the built-in Kafka, which aligns with the built-in Postgres(multicluster-global-hub-postgresql)

Note: you can define the nodepool instance with multicluster-global-hub, and all the broker instances will automatically have the suffix -kafka, if I recall correctly.

I would like to use kraft as a name, and then the kafka pod name can be kafka-kraft-0. it is easy to understand the kafka is use kraft mode.

Ok, it looks like we have the final decision. Let's go with the name kraft

yanmxa · 2024-12-19T01:17:25Z

doc/reserved_names/README.md

@@ -22,6 +22,7 @@ List all annotations are used by multicluster global hub.
 |global-hub.open-cluster-management.io/import-cluster-in-hosted=true\|false | This annotation is used to identify if managedhub cluster should be imported in hosted mode |
 | global-hub.open-cluster-management.io/with-inventory                | This annotation is used to identify the common inventory is deployed.                                                                  |
 | global-hub.open-cluster-management.io/with-stackrox-integration | This annotation enables the experimental integration with [Stackrox](https://github.com/stackrox).|
+| global-hub.open-cluster-management.io/resync-kafka-client-secret | This annotation is used to identify if the kafka client secret is resynced in agent.|


Is this annotation to indicate the agent should resign the client certificates?

Is this annotation to indicate the agent should resign the client certificates?

it means the secrete re-synced.

maybe use resign-kafka-client-secret or regenerate-kafka-client-secret

if you add the annotation, will it be removed by addon framework?

if you add the annotation, will it be removed by addon framework?

No

yanmxa · 2024-12-19T01:22:51Z

Thanks @ldpliu!

It looks good to me, just the naming issue that needs to be consistent. Leave it to @clyang82~

clyang82 · 2024-12-19T01:59:03Z

operator/pkg/controllers/transporter/protocol/manifests/global-hub-kafka-node-pool.yaml

@@ -1,7 +1,7 @@
 apiVersion: kafka.strimzi.io/v1beta2
 kind: KafkaNodePool
 metadata:
-  name: kafka
+  name: kraft


I would like to use kraft as a name, and then the kafka pod name can be kafka-kraft-0. it is easy to understand the kafka is use kraft mode.

clyang82 · 2024-12-19T02:05:41Z

operator/pkg/controllers/transporter/protocol/strimzi_transporter.go

@@ -448,7 +492,7 @@ func (k *strimziTransporter) GetConnCredential(clusterName string) (*transport.K
 	// topics
 	credential.StatusTopic = config.GetStatusTopic(clusterName)
 	credential.SpecTopic = config.GetSpecTopic()
-
+	credential.IsNewKafkaCluster = k.isNewKafkaCluster


it may have a case that the value is set as true firstly and the the operator is restarted, the value is set as false.

it may have a case that the value is set as true firstly and the the operator is restarted, the value is set as false.

I think it's fine, we only use it once. after agent resynced secret, it is useless

clyang82 · 2024-12-19T02:16:14Z

doc/reserved_names/README.md

@@ -22,6 +22,7 @@ List all annotations are used by multicluster global hub.
 |global-hub.open-cluster-management.io/import-cluster-in-hosted=true\|false | This annotation is used to identify if managedhub cluster should be imported in hosted mode |
 | global-hub.open-cluster-management.io/with-inventory                | This annotation is used to identify the common inventory is deployed.                                                                  |
 | global-hub.open-cluster-management.io/with-stackrox-integration | This annotation enables the experimental integration with [Stackrox](https://github.com/stackrox).|
+| global-hub.open-cluster-management.io/resync-kafka-client-secret | This annotation is used to identify if the kafka client secret is resynced in agent.|


maybe use resign-kafka-client-secret or regenerate-kafka-client-secret

clyang82 · 2024-12-19T02:17:18Z

doc/reserved_names/README.md

@@ -22,6 +22,7 @@ List all annotations are used by multicluster global hub.
 |global-hub.open-cluster-management.io/import-cluster-in-hosted=true\|false | This annotation is used to identify if managedhub cluster should be imported in hosted mode |
 | global-hub.open-cluster-management.io/with-inventory                | This annotation is used to identify the common inventory is deployed.                                                                  |
 | global-hub.open-cluster-management.io/with-stackrox-integration | This annotation enables the experimental integration with [Stackrox](https://github.com/stackrox).|
+| global-hub.open-cluster-management.io/resync-kafka-client-secret | This annotation is used to identify if the kafka client secret is resynced in agent.|


if you add the annotation, will it be removed by addon framework?

yanmxa · 2024-12-19T03:04:05Z

pkg/transport/controller/controller.go

+	}
+	signedSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      kafkaConn.ClientSecretName,


In the BYO case, the ClientSecretName might be empty in the kafkaConn. So, you need to assert it is not empty before retrieving the secret.

Signed-off-by: root <[email protected]>

clyang82 · 2024-12-19T06:28:37Z

pkg/transport/controller/controller.go

+	}
+	log.Infof("update transport config secret")
+
+	transportSecret.Annotations[constants.ResyncKafkaClientSecretAnnotation] = "true"


We need to confirm whether the annotation is overwritten by AppliedManifestWork or not.

confirmed. it will merge

clyang82

LGTM

openshift-ci · 2024-12-19T06:31:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clyang82, ldpliu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [clyang82]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sonarqubecloud · 2024-12-19T06:46:32Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
79.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

openshift-ci bot added the dco-signoff: yes label Dec 17, 2024

ldpliu force-pushed the kafka-upgrade branch 5 times, most recently from e6b8013 to 09f9a2d Compare December 18, 2024 06:23

yanmxa reviewed Dec 18, 2024

View reviewed changes

yanmxa reviewed Dec 19, 2024

View reviewed changes

clyang82 reviewed Dec 19, 2024

View reviewed changes

ldpliu force-pushed the kafka-upgrade branch from 09f9a2d to 675b0f7 Compare December 19, 2024 02:44

yanmxa reviewed Dec 19, 2024

View reviewed changes

handle upgrade kafka from zookeeper to kraft

b36ba67

Signed-off-by: root <[email protected]>

ldpliu force-pushed the kafka-upgrade branch from 675b0f7 to b36ba67 Compare December 19, 2024 03:16

clyang82 reviewed Dec 19, 2024

View reviewed changes

clyang82 approved these changes Dec 19, 2024

View reviewed changes

openshift-ci bot assigned clyang82 Dec 19, 2024

openshift-ci bot added the lgtm label Dec 19, 2024

openshift-ci bot added the approved label Dec 19, 2024

openshift-merge-bot bot merged commit dc440a5 into stolostron:main Dec 19, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle upgrade kafka from zookeeper to kraft #1284

handle upgrade kafka from zookeeper to kraft #1284

ldpliu commented Dec 17, 2024 •

edited

Loading

yanmxa Dec 18, 2024

ldpliu Dec 18, 2024

yanmxa Dec 18, 2024

ldpliu Dec 18, 2024

ldpliu Dec 19, 2024

yanmxa Dec 19, 2024 •

edited

Loading

yanmxa Dec 19, 2024

clyang82 Dec 19, 2024

yanmxa Dec 19, 2024

yanmxa Dec 19, 2024

ldpliu Dec 19, 2024

clyang82 Dec 19, 2024

clyang82 Dec 19, 2024

ldpliu Dec 19, 2024

yanmxa commented Dec 19, 2024

clyang82 Dec 19, 2024

clyang82 Dec 19, 2024

ldpliu Dec 19, 2024

clyang82 Dec 19, 2024

clyang82 Dec 19, 2024

yanmxa Dec 19, 2024 •

edited

Loading

clyang82 Dec 19, 2024

clyang82 Dec 19, 2024

clyang82 left a comment

openshift-ci bot commented Dec 19, 2024

sonarqubecloud bot commented Dec 19, 2024

handle upgrade kafka from zookeeper to kraft #1284

handle upgrade kafka from zookeeper to kraft #1284

Conversation

ldpliu commented Dec 17, 2024 • edited Loading

Summary

Related issue(s)

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanmxa Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanmxa commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanmxa Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clyang82 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 19, 2024

sonarqubecloud bot commented Dec 19, 2024

Quality Gate passed

ldpliu commented Dec 17, 2024 •

edited

Loading

yanmxa Dec 19, 2024 •

edited

Loading

yanmxa Dec 19, 2024 •

edited

Loading