Copilot commented on code in PR #47:
URL:
https://github.com/apache/skywalking-banyandb-helm/pull/47#discussion_r2703650366
##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -308,12 +308,11 @@ spec:
{{- end }}
args:
- --proxy-addr={{ template "banyandb.fullname" $
}}-fodc-proxy-grpc:{{ $.Values.cluster.fodc.proxy.grpcSvc.port }}
- - --node-ip=127.0.0.1
- - --node-port={{ $.Values.cluster.data.nodeTemplate.grpcSvc.port }}
+ - --pod-name=$(POD_NAME) \
+ - --container-name=2121,17915 \
Review Comment:
Lines 311-312 have trailing backslashes but the subsequent lines do not.
This is inconsistent and incorrect YAML syntax. Either all lines except the
last should have backslashes (for line continuation), or no lines should have
them. The backslashes on lines 311 and 312 should be removed to match the
pattern of the other arguments.
```suggestion
- --pod-name=$(POD_NAME)
- --container-name=2121,17915
```
##########
test/e2e/values.fodc.yaml:
##########
@@ -315,10 +315,9 @@ cluster:
resources:
requests: []
limits: []
Review Comment:
The resources field is still using the old array format (requests: [],
limits: []) but the templates have been updated to expect a map format and use
toYaml. This should be updated to match the new format used in
chart/values.yaml, either as empty objects (requests: {}, limits: {}) or with
actual resource values like limits: {memory: 256Mi}.
```suggestion
requests: {}
limits: {}
```
##########
chart/templates/cluster_liaison_statefulset.yaml:
##########
@@ -296,21 +296,16 @@ spec:
securityContext:
{{- toYaml . | nindent 12 }}
{{- end }}
- command:
- - sh
- - -c
- - |
- exec /fodc-agent \
- --proxy-addr={{ template "banyandb.fullname" .
}}-fodc-proxy-grpc:{{ .Values.cluster.fodc.proxy.grpcSvc.port }} \
- --node-ip=127.0.0.1 \
- --node-port={{ .Values.cluster.liaison.grpcSvc.port }} \
- --node-role=liaison \
- --metrics-endpoint=http://127.0.0.1:2121/metrics \
- --poll-metrics-interval={{
.Values.cluster.fodc.agent.config.scrapeInterval }} \
- --prometheus-listen-addr=:{{
.Values.cluster.fodc.agent.httpPort }} \
- --max-metrics-memory-usage-percentage={{
.Values.cluster.fodc.agent.config.ktmEnabled | ternary 10 5 }} \
- --heartbeat-interval={{
.Values.cluster.fodc.agent.config.heartbeatInterval }} \
- --reconnect-interval={{
.Values.cluster.fodc.agent.config.reconnectInterval }}
+ args:
+ - --proxy-addr={{ template "banyandb.fullname" .
}}-fodc-proxy-grpc:{{ .Values.cluster.fodc.proxy.grpcSvc.port }} \
+ - --pod-name=$(POD_NAME) \
+ - --container-name=2121 \
+ - --node-role=liaison \
+ - --poll-metrics-interval={{
.Values.cluster.fodc.agent.config.pollMetricsInterval }} \
+ - --prometheus-listen-addr=:{{
.Values.cluster.fodc.agent.metricsPort }} \
+ - --max-metrics-memory-usage-percentage={{
.Values.cluster.fodc.agent.config.ktmEnabled | ternary 10 5 }} \
+ - --heartbeat-interval={{
.Values.cluster.fodc.agent.config.heartbeatInterval }} \
+ - --reconnect-interval={{
.Values.cluster.fodc.agent.config.reconnectInterval }}
Review Comment:
All arguments have trailing backslashes including the last one. In YAML,
when using multi-line strings with backslash continuation, the last line should
NOT have a trailing backslash. The backslash on line 308 after the
reconnect-interval argument should be removed.
##########
doc/parameters.md:
##########
@@ -366,9 +366,8 @@ The content of this document describes the parameters that
can be configured in
| `cluster.fodc.agent.containerSecurityContext` | Container-level
security context for Agent | `{}`
|
| `cluster.fodc.agent.resources.requests` | Resource requests
for Agent | `[]`
|
| `cluster.fodc.agent.resources.limits` | Resource limits
for Agent | `[]`
|
-| `cluster.fodc.agent.grpcPort` | GRPC port for
Agent sidecar (not used - agent connects outbound to proxy) | `17914`
|
-| `cluster.fodc.agent.httpPort` | HTTP port for
Agent sidecar (prometheus-listen-addr flag) | `17915`
|
-| `cluster.fodc.agent.config.scrapeInterval` | Interval for
scraping BanyanDB metrics (poll-metrics-interval flag) | `15s`
|
+| `cluster.fodc.agent.metricsPort` | Metrics port for
Agent sidecar (prometheus-listen-addr flag) | `17915`
|
Review Comment:
The default value shown for metricsPort is 17915, but in the actual
values.yaml file the default has been changed to 9090. This documentation
should be updated to reflect the new default value of 9090.
```suggestion
| `cluster.fodc.agent.metricsPort` | Metrics port for
Agent sidecar (prometheus-listen-addr flag) | `9090`
|
```
##########
doc/parameters.md:
##########
@@ -366,9 +366,8 @@ The content of this document describes the parameters that
can be configured in
| `cluster.fodc.agent.containerSecurityContext` | Container-level
security context for Agent | `{}`
|
| `cluster.fodc.agent.resources.requests` | Resource requests
for Agent | `[]`
|
| `cluster.fodc.agent.resources.limits` | Resource limits
for Agent | `[]`
|
Review Comment:
The default values shown for resources.requests and resources.limits are
still showing the old array format ([]) but these have been changed to map
format in the actual values.yaml file (requests: {}, limits: {memory: 256Mi}).
This documentation should be updated to reflect the new default values.
```suggestion
| `cluster.fodc.agent.resources.requests` | Resource
requests for Agent | `{}`
|
| `cluster.fodc.agent.resources.limits` | Resource limits
for Agent | `{memory:
256Mi}` |
```
##########
chart/templates/standalone_statefulset.yaml:
##########
@@ -244,39 +244,16 @@ spec:
securityContext:
{{- toYaml . | nindent 12 }}
{{- end }}
- command:
- - sh
- - -c
- - |
- set -euo pipefail
- echo "Waiting for BanyanDB observability port (2121) to be
ready..."
- MAX_ATTEMPTS=60
- ATTEMPT=0
- while [ ${ATTEMPT} -lt ${MAX_ATTEMPTS} ]; do
- if wget -q -T 1 -O- "http://127.0.0.1:2121/metrics"; >
/dev/null 2>&1; then
- echo "BanyanDB observability port is ready, starting FODC
agent..."
- break
- fi
- ATTEMPT=$((ATTEMPT + 1))
- if [ $((ATTEMPT % 5)) -eq 0 ]; then
- echo "Waiting for BanyanDB observability port (attempt
${ATTEMPT}/${MAX_ATTEMPTS})..."
- fi
- sleep 2
- done
- if [ ${ATTEMPT} -ge ${MAX_ATTEMPTS} ]; then
- echo "Warning: BanyanDB observability port not ready after
${MAX_ATTEMPTS} attempts, starting agent anyway..."
- fi
- exec /fodc-agent \
- --proxy-addr={{ template "banyandb.fullname" .
}}-fodc-proxy-grpc:{{ .Values.cluster.fodc.proxy.grpcSvc.port }} \
- --node-ip=${POD_NAME}.banyandb.${POD_NAMESPACE} \
- --node-port={{ .Values.standalone.grpcSvc.port }} \
- --node-role=standalone \
- --metrics-endpoint=http://127.0.0.1:2121/metrics \
- --poll-metrics-interval={{
.Values.cluster.fodc.agent.config.scrapeInterval }} \
- --prometheus-listen-addr=:{{
.Values.cluster.fodc.agent.httpPort }} \
- --max-metrics-memory-usage-percentage={{
.Values.cluster.fodc.agent.config.ktmEnabled | ternary 10 5 }} \
- --heartbeat-interval={{
.Values.cluster.fodc.agent.config.heartbeatInterval }} \
- --reconnect-interval={{
.Values.cluster.fodc.agent.config.reconnectInterval }}
+ args:
+ - --proxy-addr={{ template "banyandb.fullname" .
}}-fodc-proxy-grpc:{{ .Values.cluster.fodc.proxy.grpcSvc.port }} \
+ - --pod-name=$(POD_NAME) \
+ - --container-name=2121 \
+ - --node-role=standalone \
+ - --poll-metrics-interval={{
.Values.cluster.fodc.agent.config.pollMetricsInterval }} \
+ - --prometheus-listen-addr=:{{
.Values.cluster.fodc.agent.metricsPort }} \
+ - --max-metrics-memory-usage-percentage={{
.Values.cluster.fodc.agent.config.ktmEnabled | ternary 10 5 }} \
+ - --heartbeat-interval={{
.Values.cluster.fodc.agent.config.heartbeatInterval }} \
+ - --reconnect-interval={{
.Values.cluster.fodc.agent.config.reconnectInterval }}
Review Comment:
All arguments have trailing backslashes including the last one. In YAML,
when using multi-line strings with backslash continuation, the last line should
NOT have a trailing backslash. The backslash on line 256 after the
reconnect-interval argument should be removed.
--
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]
