[ISSUE #176] Support manage multi cluster by drivebyer · Pull Request #183 · apache/rocketmq-operator

drivebyer · 2023-09-28T13:18:01Z

What is the purpose of the change

In order to manage multi cluster by one operator.

task1 && task2 of #176

Brief changelog

XX

Verifying this change

tested against old cluster()
tested against creating two cluster in one namespace
yaml1(set rocketmq cluster name to empty string): https://gist.github.com/drivebyer/8795a96966be4fd6ebf395cc347a159d
yaml2(set rocketmq cluster name to test-rocketmq): https://gist.github.com/drivebyer/338a0cba168249dd3c02ce0f33f775d8

Please go through this checklist to help us incorporate your contribution quickly and easily.

Notice: It would be helpful if you could finish the following checklist (the last one is not necessary) before request the community to review your PR.

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Check RBAC rights for Kubernetes roles.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist.
Run make docker-build to build docker image for operator, try your changes from Pod inside your Kubernetes cluster, not just locally. Also provide screenshots to show that the RocketMQ cluster is healthy after the changes.
Before committing your changes, remember to run make manifests to make sure the CRD files are updated.
Update documentation if necessary.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

drivebyer · 2023-10-30T03:18:42Z

@caigy PTAL

caigy

It seems that there're lots of modifications in this pr, you'd better add some doc for your design, especially for the changes in CRs reflecting the status of all modules in the RocketMQ cluster.

caigy · 2024-02-13T09:25:54Z

+	err := mgr.GetFieldIndexer().IndexField(context.TODO(), &rocketmqv1alpha1.NameService{}, rocketmqv1alpha1.NameServiceRocketMqNameIndexKey,
+		func(rawObj client.Object) []string {
+			n, ok := rawObj.(*rocketmqv1alpha1.NameService)
+			if !ok {
+				return nil
+			}
+			return []string{n.Spec.RocketMqName + "-" + n.Namespace}
+		},
+	)
+	if err != nil {
+		return err
+	}


What's the usage of this block? You'd better show more about your design.

caigy · 2024-02-13T09:32:37Z

+			// get cluster of output
+			clusterName := ""
+			for _, line := range strings.Split(string(clusterListOutput), "\n") {
+				if strings.HasPrefix(line, "#Cluster Name") {
+					continue
+				}


It's not stable to get cluster name by analyzing the output of a command.

caigy · 2024-02-13T09:37:24Z

+			// #Cluster Name     #Broker Name            #BID  #Addr                  #Version                #InTPS(LOAD)       #OutTPS(LOAD) #PCWait(ms) #Hour #SPACE
+			// broker            broker-0                0     192.168.180.40:10911   V4_5_0                   0.00(0,0ms)         0.00(0,0ms)          0 471030.34 -1.0000
+			// broker            broker-0                1     192.168.137.89:10911   V4_5_0                   0.00(0,0ms)         0.00(0,0ms)          0 471030.34 0.2673
+			clusterListCmd := exec.Command("sh", cons.AdminToolDir, cons.ClusterList, "-n", oldNameServerListStr)


As isNameServersStrUpdated is true, why using oldNameServerListStr as the addresses of name servers?

caigy · 2024-02-13T09:39:23Z

+			command := mqAdmin + " " + subCmd + " -c " + clusterName + " -k " + key + " -n " + oldNameServerListStr + " -v " + newNameServerListStr
+			cmd := exec.Command("sh", mqAdmin, subCmd, "-c", clusterName, "-k", key, "-n", oldNameServerListStr, "-v", newNameServerListStr)


Only brokers registering successfully to name servers can receive the command, others will not be updated.

caigy · 2024-02-13T09:41:15Z

-	runningNameServerNum := getRunningNameServersNum(podList.Items)
-	if runningNameServerNum == instance.Spec.Size {
-		share.IsNameServersStrInitialized = true
-		share.NameServersStr = nameServerListStr // reassign if operator restarts
-	}


Can this block be safely removed?

drivebyer · 2024-02-19T09:09:51Z

It seems that there're lots of modifications in this pr, you'd better add some doc for your design, especially for the changes in CRs reflecting the status of all modules in the RocketMQ cluster.

OK, i would add this.

RockteMQ-AI · 2026-07-30T10:39:52Z

⚠️ Merge conflict detected

This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts:

git fetch origin
git checkout multiple-rmqs
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.

Automated notification by github-manager-bot

RockteMQ-AI

Summary

This PR modifies 25 file(s) with 805 lines of diff. No test changes detected — consider adding test coverage.

Automated review by github-manager-bot

Additional notes (not anchored to a changed line)

[INFO] charts/rocketmq-operator/crds/rocketmq.apache.org_brokers.yaml:1 — Large diff (805 lines). Consider breaking into smaller, focused PRs for easier review. (line outside diff)

RockteMQ-AI · 2026-08-05T14:54:13Z

@@ -28,6 +28,9 @@ import (
 // BrokerSpec defines the desired state of Broker


No test changes detected alongside source modifications. Consider adding tests to cover the changes.

RockteMQ-AI

Summary

PR received and logged for review. This PR requires detailed code review by a maintainer.

Diff size: 805 lines
Author: drivebyer (CONTRIBUTOR)

Automated review by RockteMQ-AI

RockteMQ-AI

Summary

Review of PR #183: [ISSUE #176] Support manage multi cluster

Findings: 14 issue(s) identified (4 critical).
CLA: unknown

Please address the inline comments above.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-23T19:55:02Z

+	"context"
+	"sort"
+	"strings"



GetNameServersStr requires exactly 1 NameService matching the index key (len != 1 returns empty). If a user has zero or more than one NameService CR with the same rocketMqName in a namespace, the broker controller enters an infinite blocking loop (broker_controller.go busy-wait for {} loop), starving the reconcile goroutine and preventing any progress. The function should either tolerate multiple NameServices or provide a meaningful error/log.

RockteMQ-AI · 2026-08-23T19:55:02Z

+		groupNum = broker.Spec.Size
 	} else {
-		share.GroupNum = broker.Status.Size
+		groupNum = broker.Status.Size


The for {} busy-wait loop for name server readiness blocks the reconcile goroutine indefinitely with only a 2-second sleep between iterations. If the NameService never becomes ready, this blocks the controller thread forever. This should use return reconcile.Result{Requeue: true, RequeueAfter: ...}, nil instead, consistent with the pattern already used elsewhere in this same file (e.g., line 163 for controller readiness).

RockteMQ-AI · 2026-08-23T19:55:02Z

+	var nameserverStr string
 	if instance.Spec.NameServers == "" {
 		// wait for name server ready if nameServers is omitted
 		for {


Same infinite blocking loop as in the broker controller: for {} with a sleep waiting for name server readiness. This blocks the reconcile goroutine permanently if the NameService never becomes ready. Should requeue instead.

RockteMQ-AI · 2026-08-23T19:55:02Z


+func (r *ReconcileBroker) getControllerAccessPoint(namespace string, rocketMqName string) string {
+	controllerList := &rocketmqv1alpha1.ControllerList{}
+	err := r.client.List(context.TODO(), controllerList, &client.MatchingFields{


getControllerAccessPoint requires exactly 1 Controller (len != 1 returns empty). If there are 0 or 2+ Controller CRs with the same rocketMqName, this returns empty, causing the broker to requeue endlessly in CONTROLLER mode without a clear diagnostic message explaining why.

RockteMQ-AI · 2026-08-23T19:55:02Z

+	"github.com/apache/rocketmq-operator/pkg/tool"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"sigs.k8s.io/controller-runtime/pkg/client"


GetNameServersStr uses client.MatchingFields with the NameServiceRocketMqNameIndexKey, but the index is registered only in the nameservice controller's add() function (nameservice_controller.go). If GetNameServersStr is called before that index is registered (e.g., during startup ordering), the List call will fail silently (returning empty string). Similarly, the controller index in broker_controller.go uses mgr.GetCache().IndexField while nameservice uses mgr.GetFieldIndexer().IndexField — these are equivalent but the inconsistency is worth noting.

RockteMQ-AI · 2026-08-23T19:55:02Z

 kind: ServiceAccount
 metadata:
-  name: {{ template "rocketmq-operator.serviceAccountName" . }}
+  name: rocketmq-operator


The ServiceAccount name is now hardcoded to 'rocketmq-operator' instead of using the Helm template {{ template "rocketmq-operator.serviceAccountName" . }}. This removes the ability to customize the ServiceAccount name and will conflict if multiple operator instances are deployed in the same cluster.

RockteMQ-AI · 2026-08-23T19:55:02Z

+	if len(nameserviceList.Items) != 1 {
+		return ""
+	}



GetNameServersStr does not check whether all NameService pods are running before returning the name server list. It returns as soon as at least one running pod with a non-empty IP is found. Previously, IsNameServersStrInitialized was only set to true when runningNameServerNum == instance.Spec.Size (all name servers running). This change means brokers and consoles may connect to a partially-ready name server cluster, which could cause intermittent failures during initial deployment.

RockteMQ-AI · 2026-08-23T19:55:02Z

+		if isNameServersStrUpdated && (len(oldNameServerListStr) > cons.MinIpListLength) && (len(newNameServerListStr) > cons.MinIpListLength) {
+			// bash-4.4$ ./mqadmin clusterList -n 192.168.180.36:9876
+			// #Cluster Name     #Broker Name            #BID  #Addr                  #Version                #InTPS(LOAD)       #OutTPS(LOAD) #PCWait(ms) #Hour #SPACE
+			// broker            broker-0                0     192.168.180.40:10911   V4_5_0                   0.00(0,0ms)         0.00(0,0ms)          0 471030.34 -1.0000


The clusterList command is executed on the operator pod itself using exec.Command("sh", ...), which requires the RocketMQ admin tool to be installed in the operator container. Parsing its stdout is fragile — the output format could change between RocketMQ versions. If the admin tool is not available in the operator container (e.g., minimal operator image), this will fail.

RockteMQ-AI · 2026-08-23T19:55:02Z

 	sourceCluster := topicTransfer.Spec.SourceCluster

-	nameServer := strings.Split(share.NameServersStr, ";")[0]
+	nameServer := strings.Split(share.GetNameServersStr(r.client, topicTransfer.Namespace, topicTransfer.Spec.RocketMqName), ";")[0]


GetNameServersStr is called inline and its result is immediately split on ';'. If GetNameServersStr returns an empty string (e.g., no matching NameService), strings.Split("", ";")[0] returns an empty string, which then passes the length check. However, the function will silently proceed with an empty name server address rather than logging the root cause (no matching NameService found for the given rocketMqName).

RockteMQ-AI · 2026-08-23T19:55:02Z

 	if !reflect.DeepEqual(hostIps, instance.Status.NameServers) {
 		oldNameServerListStr := ""
 		for _, value := range instance.Status.NameServers {
 			oldNameServerListStr = oldNameServerListStr + value + ":9876;"


The variable newNameServerListStr is declared before the if !reflect.DeepEqual(...) block but is only assigned inside it. If the DeepEqual check passes (no update needed), newNameServerListStr remains empty string. This is not currently used after the block so it's harmless, but the declaration scope is wider than necessary.

linjiemiao and others added 9 commits September 27, 2023 09:50

eliminate share variable: broker group name

9254794

add rocketmq name field

fe34a95

eliminate share variable: controller access point

6f14b62

eliminate share variable: group number

d83e947

rocketMqName should not be required

bd2abd2

add indexer for list option

392e6bc

eliminate share variable: nameserver address related

983b6a2

fix potential panic

d8b14ea

execute make manifests

4fa2c0a

caigy requested changes Feb 13, 2024

View reviewed changes

RockteMQ-AI reviewed Aug 5, 2026

View reviewed changes

RockteMQ-AI reviewed Aug 20, 2026

View reviewed changes

RockteMQ-AI suggested changes Aug 23, 2026

View reviewed changes

		command := mqAdmin + " " + subCmd + " -c " + clusterName + " -k " + key + " -n " + oldNameServerListStr + " -v " + newNameServerListStr
		cmd := exec.Command("sh", mqAdmin, subCmd, "-c", clusterName, "-k", key, "-n", oldNameServerListStr, "-v", newNameServerListStr)

		@@ -28,6 +28,9 @@ import (
		// BrokerSpec defines the desired state of Broker

Uh oh!

Conversation

drivebyer commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief changelog

Verifying this change

Uh oh!

drivebyer commented Oct 30, 2023

Uh oh!

caigy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drivebyer commented Feb 19, 2024

Uh oh!

RockteMQ-AI commented Jul 30, 2026

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Additional notes (not anchored to a changed line)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drivebyer commented Sep 28, 2023 •

edited

Loading