Skip to content

[ISSUE #50] Feature add rocketmq exporter to nameserver - #51

Open
linjiemiao wants to merge 5 commits into
apache:masterfrom
silotrd:feature-code-review
Open

[ISSUE #50] Feature add rocketmq exporter to nameserver#51
linjiemiao wants to merge 5 commits into
apache:masterfrom
silotrd:feature-code-review

Conversation

@linjiemiao

Copy link
Copy Markdown

No description provided.

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

liuruiyiyang commented Sep 18, 2020

Copy link
Copy Markdown
Contributor

Thanks for your contribution~
There are several questions:

  1. Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?
  2. 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.
  3. 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

Copy link
Copy Markdown
Author

Thanks for your contribution~
There are several questions:

  1. Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?
  2. 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.
  3. 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

liuruiyiyang commented Sep 18, 2020

Copy link
Copy Markdown
Contributor

Thanks for your contribution~
There are several questions:

  1. Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?
  2. 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.
  3. 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.

@linjiemiao

Copy link
Copy Markdown
Author

Thanks for your contribution~
There are several questions:

  1. Design. Since the exporter is an independent service, would it be better to deploy in an independent pod and be optional?
  2. 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.
  3. 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

@liuruiyiyang liuruiyiyang added the enhancement New feature or request label Sep 20, 2020
@liuruiyiyang liuruiyiyang changed the title Feature add rocketmq exporter to nameserver [ISSUE #50] Feature add rocketmq exporter to nameserver Sep 20, 2020
- name: ROCKETMQ_VERSION
value: V4_3_2
- name: NAMESRV_ADDR
value: 127.0.0.1:9876

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RockteMQ-AI

Copy link
Copy Markdown

⚠️ 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 RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The PR adds useful exporter sidecar functionality but has a broken build script (inverted return codes), a CRD schema with no sub-field validation, missing container probes/ports, a non-reproducible Dockerfile, and a personal Docker image in the example — all of which should be addressed before merge.

Findings

  • [CRITICAL] deploy/crds/rocketmq_v1alpha1_nameservice_crd.yaml:66 — 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.
  • [WARNING] pkg/apis/rocketmq/v1alpha1/nameservice_types.go:59 — 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.
  • [WARNING] pkg/controller/nameservice/nameservice_controller.go:365 — 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.
  • [WARNING] pkg/controller/nameservice/nameservice_controller.go:357 — 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.
  • [WARNING] pkg/controller/nameservice/nameservice_controller.go:357 — 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).
  • [WARNING] images/rocketmq-exporter/alpine/Dockerfile:36 — 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.
  • [INFO] images/rocketmq-exporter/alpine/Dockerfile:21 — 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.
  • [CRITICAL] images/rocketmq-exporter/alpine/build-rocketmq-exporter-image.sh:24 — 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.
  • [WARNING] example/rocketmq_v1alpha1_rocketmq_exporter_cluster.yaml:140 — 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.
  • [INFO] pkg/controller/nameservice/nameservice_controller.go:349 — 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'.
  • [INFO] pkg/controller/nameservice/nameservice_controller.go:345 — 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.

Automated review by github-manager-bot

items:
type: object
type: array
exporter:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// RocketmqExporter defines the specification for the rocketmq exporter
type RocketmqExporter struct {
Enabled bool `json:"enabled,omitempty"`
Image string `json:"image,omitempty"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if specPolicy == "" {
return corev1.PullAlways
}
return specPolicy

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (r *ReconcileNameService) createRocketMQExporterContainer(nameService *rocketmqv1alpha1.NameService) (container corev1.Container) {
container = corev1.Container{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (r *ReconcileNameService) createRocketMQExporterContainer(nameService *rocketmqv1alpha1.NameService) (container corev1.Container) {
container = corev1.Container{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


RUN apk add --no-cache bash gettext nmap-ncat curl git openssl busybox-extras

ARG MAVEN_VERSION=3.6.3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

echo $1 |grep -E "^[0-9]+\.[0-9]+\.[0-9]+" > /dev/null
if [ $? = 0 ]; then
return 1
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value: 127.0.0.1:9876
image: miaolinjie/rocketmq-exporter:latest
imagePullPolicy: Always
resources:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if nameService.Spec.Exporter.Enabled {
exporter := r.createRocketMQExporterContainer(nameService)
dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, exporter)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

7 finding(s) to address.

Findings

  • [WARNING] deploy/crds/rocketmq_v1alpha1_nameservice_crd.yaml:68 — The CRD declares exporter as an unstructured object. On clusters that prune unknown CRD fields, enabled/image/env/resources will be removed, so reconciliation always sees Enabled=false. Define the nested schema (or explicitly preserve unknown fields).
  • [WARNING] pkg/apis/rocketmq/v1alpha1/nameservice_types.go:53 — Exporter annotations from the example cannot work: RocketmqExporter has no Annotations field and the controller never copies them to the Pod template. Prometheus discovery configured in the example will not select these pods.
  • [WARNING] pkg/controller/nameservice/nameservice_controller.go:370 — Providing CPU-only resource settings causes this branch to replace the entire ResourceRequirements with defaults, silently discarding the user's CPU values. Default missing resource keys individually, or only default when both resource lists are empty.
  • [WARNING] images/rocketmq-exporter/alpine/application.yml:17 — rocketmqVersion is hard-coded to V4_3_2, so the ROCKETMQ_VERSION environment variable supplied by the example is ignored. This can make exporter commands incompatible with the deployed RocketMQ version.
  • [WARNING] images/rocketmq-exporter/alpine/exporterStart.sh:18 — java is not exec'd, so PID 1 is bash and SIGTERM may not reach the JVM during Pod termination. Use exec java so the exporter can shut down gracefully.
  • [WARNING] images/rocketmq-exporter/alpine/Dockerfile:40 — The image build clones the exporter default branch without pinning a revision; the version build argument is not declared or used. Version-tagged images are therefore non-reproducible and may contain an incompatible exporter release.
  • [INFO] pkg/apis/rocketmq/v1alpha1/nameservice_types.go:53 — The new Exporter field contains slices, pointers, and resource maps, but no regenerated deepcopy implementation is included. Existing generated DeepCopy methods will shallow-copy these fields, violating Kubernetes API object copy semantics.

Automated review by github-manager-bot

type: array
exporter:
description: Exporter defines rocketmq metrics
type: object

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CRD declares exporter as an unstructured object. On clusters that prune unknown CRD fields, enabled/image/env/resources will be removed, so reconciliation always sees Enabled=false. Define the nested schema (or explicitly preserve unknown fields).

// VolumeClaimTemplates defines the StorageClass
VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates"`
// rocketmq exporter
Exporter RocketmqExporter `json:"exporter,omitempty"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exporter annotations from the example cannot work: RocketmqExporter has no Annotations field and the controller never copies them to the Pod template. Prometheus discovery configured in the example will not select these pods.

Resources: func(requirements corev1.ResourceRequirements) corev1.ResourceRequirements {
if requirements.Limits.Memory().IsZero() && requirements.Requests.Memory().IsZero() {
return corev1.ResourceRequirements{
Limits: corev1.ResourceList{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providing CPU-only resource settings causes this branch to replace the entire ResourceRequirements with defaults, silently discarding the user's CPU values. Default missing resource keys individually, or only default when both resource lists are empty.

rocketmq:
config:
webTelemetryPath: ${WEB_TELEMETRY_PATH}
rocketmqVersion: V4_3_2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rocketmqVersion is hard-coded to V4_3_2, so the ROCKETMQ_VERSION environment variable supplied by the example is ignored. This can make exporter commands incompatible with the deployed RocketMQ version.

# See the License for the specific language governing permissions and
# limitations under the License.

java -jar /rocketmq-exporter/rocketmq-exporter.jar --spring.config.location=/rocketmq-exporter/application.yml

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java is not exec'd, so PID 1 is bash and SIGTERM may not reach the JVM during Pod termination. Use exec java so the exporter can shut down gracefully.

&& ln -s /usr/share/maven/bin/mvn /usr/bin/mvn

RUN mkdir /exporter \
&& git clone https://github.com/apache/rocketmq-exporter.git \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image build clones the exporter default branch without pinning a revision; the version build argument is not declared or used. Version-tagged images are therefore non-reproducible and may contain an incompatible exporter release.

// VolumeClaimTemplates defines the StorageClass
VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates"`
// rocketmq exporter
Exporter RocketmqExporter `json:"exporter,omitempty"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new Exporter field contains slices, pointers, and resource maps, but no regenerated deepcopy implementation is included. Existing generated DeepCopy methods will shallow-copy these fields, violating Kubernetes API object copy semantics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants