RockteMQ-AI commented on code in PR #51:
URL: https://github.com/apache/rocketmq-operator/pull/51#discussion_r3839470827
##########
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"`
+ ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"`
Review Comment:
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.
##########
deploy/crds/rocketmq_v1alpha1_nameservice_crd.yaml:
##########
@@ -63,6 +63,9 @@ spec:
items:
type: object
type: array
Review Comment:
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.
##########
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
+ }(nameService.Spec.Exporter.ImagePullPolicy),
+ Resources: func(requirements corev1.ResourceRequirements)
corev1.ResourceRequirements {
+ if requirements.Limits.Memory().IsZero() &&
requirements.Requests.Memory().IsZero() {
Review Comment:
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.
##########
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
Review Comment:
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'.
##########
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
+ }(nameService.Spec.Exporter.ImagePullPolicy),
+ Resources: func(requirements corev1.ResourceRequirements)
corev1.ResourceRequirements {
+ if requirements.Limits.Memory().IsZero() &&
requirements.Requests.Memory().IsZero() {
Review Comment:
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.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -342,8 +343,42 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
VolumeClaimTemplates:
getVolumeClaimTemplates(nameService),
},
Review Comment:
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.
##########
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
Review Comment:
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.
##########
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 \
+ && ln -s /usr/share/maven/bin/mvn /usr/bin/mvn
+
+RUN mkdir /exporter \
+ && git clone https://github.com/apache/rocketmq-exporter.git \
+ && cd /rocketmq-exporter \
+ && mvn clean package -Dmaven.test.skip=true \
+ && mv /rocketmq-exporter/target/rocketmq-exporter-*.jar
/rocketmq-exporter/rocketmq-exporter.jar \
Review Comment:
The Dockerfile clones rocketmq-exporter from
`https://github.com/apache/rocketmq-exporter.git` at HEAD without pinning to a
specific tag or commit SHA. This makes image builds non-reproducible and
introduces a supply chain risk — a future upstream commit could break the build
or introduce malicious code. Pin to a specific release tag or commit SHA.
##########
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'.
Review Comment:
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.
##########
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)
Review Comment:
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.
##########
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 \
Review Comment:
The SHA512 checksum value for the Maven binary (`ARG SHA=c35a1803...`)
appears truncated — a valid SHA-512 hex string is 128 characters, but the value
here is shorter. If the checksum is wrong, the `sha512sum -c` check will fail
at build time, breaking image builds. Verify and provide the correct full
SHA-512 hash.
##########
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"`
+}
Review Comment:
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.
--
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]
