[ISSUE #50] Feature add rocketmq exporter to nameserver by linjiemiao · Pull Request #51 · apache/rocketmq-operator

linjiemiao · 2020-09-17T09:39:32Z

No description provided.

feature add rocketmq-exporter to cluster

liuruiyiyang · 2020-09-18T04:08:02Z

Thanks for your contribution~
There are several questions:

Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?
Docker image. The image used by rocketmq-operator is better also open-sourced. So the dockerfile and other image related code should be added to our image package(dir) in general (or at least add some docs about how to build it). In that way we can build it, test it and push it to our official repository apacherocketmq.
Documentation. It is better to add some documents/related links about how to use it in full functioning. In fact there are dependencies like Prometheus and Grafana.

linjiemiao · 2020-09-18T05:31:31Z

Thanks for your contribution~
There are several questions:

Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?

Docker image. The image used by rocketmq-operator is better also open-sourced. So the dockerfile and other image related code should be added to our image package(dir) in general (or at least add some docs about how to build it). In that way we can build it, test it and push it to our official repository apacherocketmq.

Documentation. It is better to add some documents/related links about how to use it in full functioning. In fact there are dependencies like Prometheus and Grafana.

Design：

If the exporter is used as an independent pod, when I have multiple nameservers, my exporter does not know which nameserver to connect, so I use exporter as an option of nameserver, deploy it as a sidecar to nameserver。Can you give me some design advice?

liuruiyiyang · 2020-09-18T07:48:05Z

Thanks for your contribution~
There are several questions:

Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?

Docker image. The image used by rocketmq-operator is better also open-sourced. So the dockerfile and other image related code should be added to our image package(dir) in general (or at least add some docs about how to build it). In that way we can build it, test it and push it to our official repository apacherocketmq.

Documentation. It is better to add some documents/related links about how to use it in full functioning. In fact there are dependencies like Prometheus and Grafana.

Design：

If the exporter is used as an independent pod, when I have multiple nameservers, my exporter does not know which nameserver to connect, so I use exporter as an option of nameserver, deploy it as a sidecar to nameserver。Can you give me some design advice?

In fact operator maintains a variable in the shared package named NameServersStr which is the NAMESRV_ADDR like: 192.168.1.1:9876;192.168.1.2:9876. You can check the code and use that.

2) add how to user rocketmq-exporter

…etmq-operator into feature-code-review

linjiemiao · 2020-09-18T08:36:56Z

Thanks for your contribution~
There are several questions:

Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?

Docker image. The image used by rocketmq-operator is better also open-sourced. So the dockerfile and other image related code should be added to our image package(dir) in general (or at least add some docs about how to build it). In that way we can build it, test it and push it to our official repository apacherocketmq.

Documentation. It is better to add some documents/related links about how to use it in full functioning. In fact there are dependencies like Prometheus and Grafana.

Design：
If the exporter is used as an independent pod, when I have multiple nameservers, my exporter does not know which nameserver to connect, so I use exporter as an option of nameserver, deploy it as a sidecar to nameserver。Can you give me some design advice?

In fact operator maintains a variable in the shared package named NameServersStr which is the NAMESRV_ADDR like: 192.168.1.1:9876;192.168.1.2:9876. You can check the code and use that.

ADD exporter image and document.

Please check it. I tend to use the exporter as the sidecar of the nameserver. Please check the exporter's dockerfile.

Because I tend to adjust the fields in the share package to private, so that multiple clusters can be deployed using one operator.

If you still want to deploy the exporter separately, I will do as you say.

thanks

anguoming · 2024-05-10T02:46:55Z

+      - name: ROCKETMQ_VERSION
+        value: V4_3_2
+      - name: NAMESRV_ADDR
+        value: 127.0.0.1:9876


I think it better to init the NAMESRV_ADDR env in nameservice_controller.go

RockteMQ-AI · 2026-07-30T10:41:01Z

⚠️ 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 feature-code-review
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 10 file(s) with 670 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] README.md:1 — Large diff (670 lines). Consider breaking into smaller, focused PRs for easier review. (line outside diff)

RockteMQ-AI · 2026-08-05T14:59:23Z

@@ -49,6 +49,17 @@ type NameServiceSpec struct {
 	HostPath string `json:"hostPath"`


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: 670 lines
Author: linjiemiao (NONE)

Automated review by RockteMQ-AI

RockteMQ-AI

Summary

Review of PR #51: [ISSUE #50] Feature add rocketmq exporter to nameserver

Findings: 12 issue(s) identified (1 critical).
CLA: unknown

Please address the inline comments above.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-23T20:05:49Z

+type RocketmqExporter struct {
+	Enabled bool `json:"enabled,omitempty"`
+	Image string `json:"image,omitempty"`
+	ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"`


RocketmqExporter.Env is tagged json:"env" (no omitempty), meaning it is a required field in JSON serialization. This will cause deserialization errors or validation failures when users omit the env field. It should be json:"env,omitempty" to be consistent with the other optional fields and match the intent of an optional exporter config.

RockteMQ-AI · 2026-08-23T20:05:49Z

+			return specPolicy
+		}(nameService.Spec.Exporter.ImagePullPolicy),
+		Resources: func(requirements corev1.ResourceRequirements) corev1.ResourceRequirements {
+			if requirements.Limits.Memory().IsZero() && requirements.Requests.Memory().IsZero() {


The resource default logic checks requirements.Limits.Memory().IsZero() && requirements.Requests.Memory().IsZero(), but if the user specifies CPU limits/requests without memory (or vice versa), the entire requirements struct is replaced with defaults, silently discarding the user's CPU configuration. The check should be done per-resource and only fill in missing individual values, not replace the whole struct.

RockteMQ-AI · 2026-08-23T20:05:49Z

+			return specPolicy
+		}(nameService.Spec.Exporter.ImagePullPolicy),
+		Resources: func(requirements corev1.ResourceRequirements) corev1.ResourceRequirements {
+			if requirements.Limits.Memory().IsZero() && requirements.Requests.Memory().IsZero() {


Calling requirements.Limits.Memory() on a nil ResourceList (when Limits is not set at all) will panic. If nameService.Spec.Exporter.Resources.Limits is nil, .Memory() returns a zero quantity via the ResourceList getter, but requirements.Limits itself being nil means the map access is safe in Go — however requirements.Requests being nil is the same case. This is safe in current Go k8s API, but the dual-nil check should be made explicit (check len(requirements.Limits) == 0 && len(requirements.Requests) == 0) to make intent clear and guard against future regressions.

RockteMQ-AI · 2026-08-23T20:05:49Z

 	HostPath string `json:"hostPath"`
 	// VolumeClaimTemplates defines the StorageClass
 	VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates"`
+	// rocketmq exporter


The Exporter field uses a value type (RocketmqExporter) rather than a pointer (*RocketmqExporter). Combined with omitempty, a zero-value struct won't be omitted in JSON because omitempty only omits zero values for scalar types and pointers in Go. Users who don't specify an exporter will still have an empty struct serialized, and the Enabled bool defaulting to false is the only guard. Using *RocketmqExporter would make the omission semantics correct and allow distinguishing 'not set' from 'set but disabled'.

RockteMQ-AI · 2026-08-23T20:05:49Z

@@ -63,6 +63,9 @@ spec:
              items:
                type: object
              type: array


The CRD schema defines exporter as type: object with no properties specified. This means Kubernetes will not validate any of the exporter subfields (enabled, image, env, resources, etc.). The schema should be expanded with proper property definitions to enable server-side validation and prevent misconfigured exporter specs from being accepted silently.

RockteMQ-AI · 2026-08-23T20:05:49Z

+  #  To have DNS options set along with hostNetwork, you have to specify DNS policy
+  #  explicitly to 'ClusterFirstWithHostNet'.
+  dnsPolicy: ClusterFirstWithHostNet
+  # resources describes the compute resource requirements and limits


The example file uses image: miaolinjie/rocketmq-exporter:latest (a personal DockerHub repo) while the README example and build script reference apacherocketmq/rocketmq-exporter. Using a personal/unofficial image in example manifests is a supply chain concern and will confuse users. The example should reference the official Apache image or the image built by the provided Dockerfile.

RockteMQ-AI · 2026-08-23T20:05:49Z

+  hostNetwork: true
+  #  Set DNS policy for the pod.
+  #  Defaults to "ClusterFirst".
+  #  Valid values are 'ClusterFirstWithHostNet', 'ClusterFirst', 'Default' or 'None'.


The exporter sidecar uses NAMESRV_ADDR: 127.0.0.1:9876 (localhost). This only works correctly when the nameserver pod uses hostNetwork: true (which this example does set). However, the documentation does not make this dependency explicit. If a user enables the exporter without hostNetwork, the exporter will fail to reach the nameserver. This constraint should be documented or enforced/validated in the controller.

RockteMQ-AI · 2026-08-23T20:05:49Z

@@ -342,8 +343,42 @@ func (r *ReconcileNameService) statefulSetForNameService(nameService *rocketmqv1
 			VolumeClaimTemplates: getVolumeClaimTemplates(nameService),
 		},


When nameService.Spec.Exporter.Enabled is toggled from true to false on an existing StatefulSet, the reconciler will not remove the exporter container from the pod template because the existing StatefulSet update path is not shown to handle container list diffing. Verify that the StatefulSet update logic compares and removes containers when the exporter is disabled, otherwise the sidecar will persist until the StatefulSet is deleted and recreated.

RockteMQ-AI · 2026-08-23T20:05:49Z

+		dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, exporter)
+	}
 	// Set Broker instance as the owner and controller
 	controllerutil.SetControllerReference(nameService, dep, r.scheme)


No test coverage is added for createRocketMQExporterContainer or the exporter injection logic in statefulSetForNameService. Given the resource defaulting logic has edge cases (partial resources, nil maps), unit tests should cover: exporter disabled, exporter enabled with full resources, exporter enabled with no resources (defaults applied), and exporter enabled with partial resources.

RockteMQ-AI · 2026-08-23T20:05:49Z

 	VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates"`
+	// rocketmq exporter
+	Exporter RocketmqExporter `json:"exporter,omitempty"`
+}


The RocketmqExporter struct has no Annotations field, but the example YAML (rocketmq_v1alpha1_rocketmq_exporter_cluster.yaml) includes annotations under the exporter spec (for prometheus scraping). These annotations will be silently dropped during deserialization since the type doesn't define them. Either add an Annotations map[string]string field to RocketmqExporter and propagate them to the pod template metadata, or remove them from the example.

linjiemiao and others added 2 commits September 17, 2020 16:55

feature add rocketmq-exporter to cluster

cbd0f7a

Merge pull request #1 from silotrd/featureadd-exporter

aa26794

feature add rocketmq-exporter to cluster

linjiemiao changed the title ~~Feature add rocketmq exporter to~~ Feature add rocketmq exporter to nameserver Sep 17, 2020

linjiemiao added 2 commits September 18, 2020 16:25

1) add rocketmq-exporter docker image (Dockerfile)

a50a2a7

2) add how to user rocketmq-exporter

Merge branch 'feature-code-review' of https://github.com/silotrd/rock…

faa70cb

…etmq-operator into feature-code-review

liuruiyiyang added the enhancement New feature or request label Sep 20, 2020

liuruiyiyang changed the title ~~Feature add rocketmq exporter to nameserver~~ [ISSUE #50] Feature add rocketmq exporter to nameserver Sep 20, 2020

Modify the way to get annotations of the pod

9f32992

anguoming reviewed May 10, 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

		@@ -49,6 +49,17 @@ type NameServiceSpec struct {
		HostPath string `json:"hostPath"`

		@@ -342,8 +343,42 @@ func (r ReconcileNameService) statefulSetForNameService(nameService rocketmqv1
		VolumeClaimTemplates: getVolumeClaimTemplates(nameService),
		},

Uh oh!

Conversation

linjiemiao commented Sep 17, 2020

Uh oh!

liuruiyiyang commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linjiemiao commented Sep 18, 2020

Uh oh!

liuruiyiyang commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linjiemiao commented Sep 18, 2020

Uh oh!

Choose a reason for hiding this comment

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!

5 participants

liuruiyiyang commented Sep 18, 2020 •

edited

Loading

liuruiyiyang commented Sep 18, 2020 •

edited

Loading