RockteMQ-AI commented on code in PR #51:
URL: https://github.com/apache/rocketmq-operator/pull/51#discussion_r3902509290
##########
pkg/apis/rocketmq/v1alpha1/nameservice_types.go:
##########
@@ -49,6 +49,17 @@ type NameServiceSpec struct {
HostPath string `json:"hostPath"`
// VolumeClaimTemplates defines the StorageClass
VolumeClaimTemplates []corev1.PersistentVolumeClaim
`json:"volumeClaimTemplates"`
+ // rocketmq exporter
+ Exporter RocketmqExporter `json:"exporter,omitempty"`
+}
+
+// RocketmqExporter defines the specification for the rocketmq exporter
+type RocketmqExporter struct {
+ Enabled bool `json:"enabled,omitempty"`
+ Image string `json:"image,omitempty"`
Review Comment:
The `Env` field is missing `omitempty` in its JSON tag (`json:"env"`),
making it effectively required for deserialization. However, the CRD schema
does not enforce this. If a user enables the exporter without specifying `env`,
the controller may encounter unexpected behavior. Add `omitempty` if env is
optional, or add CRD validation if it is required.
##########
deploy/crds/rocketmq_v1alpha1_nameservice_crd.yaml:
##########
@@ -63,6 +63,9 @@ spec:
items:
type: object
type: array
+ exporter:
Review Comment:
The `exporter` field is defined as `type: object` with no sub-schema
properties. This means Kubernetes will accept any arbitrary data under
`exporter` without validation. The full sub-schema (enabled, image,
imagePullPolicy, env, resources) should be declared here to match the Go type
`RocketmqExporter` and provide proper CRD validation at admission time.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -342,8 +343,42 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
VolumeClaimTemplates:
getVolumeClaimTemplates(nameService),
},
}
+ if nameService.Spec.Exporter.Enabled {
+ exporter := r.createRocketMQExporterContainer(nameService)
+ 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)
return dep
}
+
+func (r *ReconcileNameService) createRocketMQExporterContainer(nameService
*rocketmqv1alpha1.NameService) (container corev1.Container) {
+ container = corev1.Container{
Review Comment:
No readiness or liveness probe is configured for the exporter sidecar. If
the exporter process hangs or crashes, Kubernetes will not detect the failure
or restart the container. Add at minimum an HTTP readiness probe against the
metrics endpoint (e.g., `GET /metrics` on port 5557).
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -342,8 +343,42 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
VolumeClaimTemplates:
getVolumeClaimTemplates(nameService),
},
}
+ if nameService.Spec.Exporter.Enabled {
+ exporter := r.createRocketMQExporterContainer(nameService)
+ 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)
return dep
}
+
+func (r *ReconcileNameService) createRocketMQExporterContainer(nameService
*rocketmqv1alpha1.NameService) (container corev1.Container) {
+ container = corev1.Container{
Review Comment:
No container port is defined on the exporter sidecar. The exporter listens
on port 5557 (per `application.yml`), and the example YAML uses
`prometheus.io/port: "5557"` annotations. Adding `Ports:
[]corev1.ContainerPort{{ContainerPort: 5557, Name: "metrics"}}` improves
discoverability and enables Service-based scraping, not just annotation-based
pod scraping.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -342,8 +343,42 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
VolumeClaimTemplates:
getVolumeClaimTemplates(nameService),
},
}
+ if nameService.Spec.Exporter.Enabled {
+ exporter := r.createRocketMQExporterContainer(nameService)
+ 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)
return dep
}
+
+func (r *ReconcileNameService) createRocketMQExporterContainer(nameService
*rocketmqv1alpha1.NameService) (container corev1.Container) {
+ container = corev1.Container{
+ Name: cons.ExporterContainerName,
+ Image: nameService.Spec.Exporter.Image,
+ Env: nameService.Spec.Exporter.Env,
+ ImagePullPolicy: func(specPolicy corev1.PullPolicy)
corev1.PullPolicy {
+ if specPolicy == "" {
+ return corev1.PullAlways
+ }
+ return specPolicy
Review Comment:
The resource-defaults logic only falls back to defaults when *both*
`Limits.Memory()` and `Requests.Memory()` are zero. If a user provides only CPU
resources (or only one of requests/limits for memory), the defaults are skipped
and the user-provided partial spec is used as-is, potentially leaving memory
unconstrained. Consider checking all four resource dimensions independently.
##########
images/rocketmq-exporter/alpine/Dockerfile:
##########
@@ -0,0 +1,51 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+FROM openjdk:8-alpine
+
+RUN apk add --no-cache bash gettext nmap-ncat curl git openssl busybox-extras
+
+ARG MAVEN_VERSION=3.6.3
+ARG BASE_URL=https://apache.osuosl.org/maven/maven-3/${MAVEN_VERSION}/binaries
+ARG
SHA=c35a1803a6e70a126e80b2b3ae33eed961f83ed74d18fcd16909b2d44d7dada3203f1ffe726c17ef8dcca2dcaa9fca676987befeadc9b9f759967a8cb77181c0
+
+RUN mkdir -p /usr/share/maven /usr/share/maven/ref \
+ && echo "Downlaoding maven" \
+ && curl -fsSL -o /tmp/apache-maven.tar.gz
${BASE_URL}/apache-maven-${MAVEN_VERSION}-bin.tar.gz \
+ \
+ && echo "Checking download hash" \
+ && echo "${SHA} /tmp/apache-maven.tar.gz" | sha512sum -c - \
+ \
+ && echo "Unziping maven" \
+ && tar -xzf /tmp/apache-maven.tar.gz -C /usr/share/maven
--strip-components=1 \
+ \
+ && echo "Cleaning and setting links" \
+ && rm -f /tmp/apache-maven.tar.gz \
Review Comment:
The Dockerfile clones the `rocketmq-exporter` repo at HEAD without pinning
to a specific tag or commit SHA. This makes builds non-reproducible — the same
Dockerfile can produce different binaries over time. Pin to a specific release
tag (e.g., `git clone --branch v0.0.2 --depth 1`). Also, building from source
inside the Dockerfile bloats the image with Maven, Git, and the full JDK.
Consider a multi-stage build or a pre-built release JAR.
##########
example/rocketmq_v1alpha1_rocketmq_exporter_cluster.yaml:
##########
@@ -0,0 +1,157 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: broker-config
+data:
+ # BROKER_MEM sets the broker JVM, if set to "" then Xms = Xmx = max(min(1/2
ram, 1024MB), min(1/4 ram, 8GB))
+ BROKER_MEM: " -Xms2g -Xmx2g -Xmn1g "
+ broker-common.conf: |
+ # brokerClusterName, brokerName, brokerId are automatically generated by
the operator and do not set it manually!!!
+ deleteWhen=04
+ fileReservedTime=48
+ flushDiskType=ASYNC_FLUSH
+ # set brokerRole to ASYNC_MASTER or SYNC_MASTER. DO NOT set to SLAVE
because the replica instance will automatically be set!!!
+ brokerRole=ASYNC_MASTER
+
+---
+apiVersion: rocketmq.apache.org/v1alpha1
+kind: Broker
+metadata:
+ # name of broker cluster
+ name: broker
+spec:
+ # size is the number of the broker cluster, each broker cluster contains a
master broker and [replicaPerGroup] replica brokers.
+ size: 1
+ # nameServers is the [ip:port] list of name service
+ nameServers: ""
+ # replicaPerGroup is the number of each broker cluster
+ replicaPerGroup: 0
+ # brokerImage is the customized docker image repo of the RocketMQ broker
+ brokerImage: apacherocketmq/rocketmq-broker:4.5.0-alpine-operator-0.3.0
+ # imagePullPolicy is the image pull policy
+ imagePullPolicy: Always
+ # resources describes the compute resource requirements and limits
+ resources:
+ requests:
+ memory: "2048Mi"
+ cpu: "250m"
+ limits:
+ memory: "12288Mi"
+ cpu: "500m"
+ # allowRestart defines whether allow pod restart
+ allowRestart: true
+ # storageMode can be EmptyDir, HostPath, StorageClass
+ storageMode: EmptyDir
+ # hostPath is the local path to store data
+ hostPath: /tmp/data/rocketmq/broker
+ # scalePodName is [Broker name]-[broker group number]-master-0
+ scalePodName: broker-0-master-0
+ # env defines custom env, e.g. BROKER_MEM
+ env:
+ - name: BROKER_MEM
+ valueFrom:
+ configMapKeyRef:
+ name: broker-config
+ key: BROKER_MEM
+ # volumes defines the broker.conf
+ volumes:
+ - name: broker-config
+ configMap:
+ name: broker-config
+ items:
+ - key: broker-common.conf
+ path: broker-common.conf
+ # volumeClaimTemplates defines the storageClass
+ volumeClaimTemplates:
+ - metadata:
+ name: broker-storage
+ spec:
+ accessModes:
+ - ReadWriteOnce
+ resources:
+ requests:
+ storage: 8Gi
+ selector:
+ matchLabels:
+ app: broker-storage-pv
+---
+apiVersion: rocketmq.apache.org/v1alpha1
+kind: NameService
+metadata:
+ name: name-service
+spec:
+ # size is the the name service instance number of the name service cluster
+ size: 1
+ # nameServiceImage is the customized docker image repo of the RocketMQ name
service
+ nameServiceImage:
apacherocketmq/rocketmq-nameserver:4.5.0-alpine-operator-0.3.0
+ # imagePullPolicy is the image pull policy
+ imagePullPolicy: Always
+ # hostNetwork can be true or false
+ hostNetwork: true
+ # Set DNS policy for the pod.
+ # Defaults to "ClusterFirst".
+ # Valid values are 'ClusterFirstWithHostNet', 'ClusterFirst', 'Default' or
'None'.
+ # DNS parameters given in DNSConfig will be merged with the policy selected
with DNSPolicy.
+ # 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
+ resources:
+ requests:
+ memory: "512Mi"
+ cpu: "250m"
+ limits:
+ memory: "1024Mi"
+ cpu: "500m"
+ # storageMode can be EmptyDir, HostPath, StorageClass
+ storageMode: EmptyDir
+ # hostPath is the local path to store data
+ hostPath: /data/rocketmq/nameserver
+ exporter:
+ enabled: true
+ annotations:
+ prometheus.io/path: /metrics
+ prometheus.io/port: "5557"
+ prometheus.io/scrape: "true"
+ env:
+ - name: WEB_TELEMETRY_PATH
+ value: /metrics
+ - name: ROCKETMQ_VERSION
+ value: V4_3_2
+ - name: NAMESRV_ADDR
+ value: 127.0.0.1:9876
+ image: miaolinjie/rocketmq-exporter:latest
+ imagePullPolicy: Always
+ resources:
Review Comment:
The example uses a personal Docker Hub image
(`miaolinjie/rocketmq-exporter:latest`) instead of the official
`apacherocketmq/rocketmq-exporter` registry used elsewhere in this project.
This should be updated to the official image before merge, and `:latest` should
be replaced with a pinned version tag for reproducibility.
##########
images/rocketmq-exporter/alpine/Dockerfile:
##########
@@ -0,0 +1,51 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+FROM openjdk:8-alpine
+
+RUN apk add --no-cache bash gettext nmap-ncat curl git openssl busybox-extras
+
+ARG MAVEN_VERSION=3.6.3
Review Comment:
Several packages installed here (`openssl`, `busybox-extras`, `git`) appear
unnecessary for running the exporter JAR at runtime. `git` is only needed for
the build step — consider a multi-stage Dockerfile to keep the final image
minimal and reduce the attack surface.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -342,8 +343,42 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
VolumeClaimTemplates:
getVolumeClaimTemplates(nameService),
},
}
Review Comment:
No test coverage for the new exporter sidecar injection logic. The
`createRocketMQExporterContainer` function and the conditional injection path
in `statefulSetForNameService` should have unit tests covering: (1) exporter
enabled with full spec, (2) exporter enabled with empty/default resources, (3)
exporter disabled, (4) custom imagePullPolicy vs empty.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -342,8 +343,42 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
VolumeClaimTemplates:
getVolumeClaimTemplates(nameService),
},
}
+ if nameService.Spec.Exporter.Enabled {
+ exporter := r.createRocketMQExporterContainer(nameService)
+ dep.Spec.Template.Spec.Containers =
append(dep.Spec.Template.Spec.Containers, exporter)
+ }
Review Comment:
The comment on line 349 says 'Set Broker instance as the owner and
controller' but the code sets the NameService as the owner. This is a
pre-existing misleading comment, but since this PR touches this area, consider
correcting it to 'Set NameService instance as the owner and controller'.
##########
images/rocketmq-exporter/alpine/build-rocketmq-exporter-image.sh:
##########
@@ -0,0 +1,43 @@
+#!/bin/bash
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+checkVersion()
+{
+ echo "Version = $1"
+ echo $1 |grep -E "^[0-9]+\.[0-9]+\.[0-9]+" > /dev/null
+ if [ $? = 0 ]; then
+ return 1
+ fi
Review Comment:
The `checkVersion` function has inverted return codes. It returns 1
(failure) when the version IS valid, and falls through to `exit 2` when
invalid. The caller `if [ $? = 0 ]` checks for success (0), which is never
returned. This means every valid version (e.g., `4.5.0`) triggers the error
message and `exit 2`. The return should be `return 0` on match, or the caller
should check `$? != 0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
