hanahmily commented on code in PR #45:
URL: 
https://github.com/apache/skywalking-banyandb-helm/pull/45#discussion_r2684717554


##########
chart/values.yaml:
##########
@@ -734,6 +737,206 @@ cluster:
         ##
         failureThreshold: 60
 
+  ## @section Configuration for FODC (First Occurrence Data Collection) Proxy 
component
+  ##
+  fodcProxy:

Review Comment:
   ```
   fodc:
      enabled: true
      agent:
          ....
      proxy:
         ....
   ```
   
   I prefer the above structure because the agent and proxy should be deployed 
simultaneously.



##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -289,6 +308,108 @@ spec:
             {{- end }}
             {{- end }}
           {{- end }}
+        {{- if and $.Values.cluster.enabled $.Values.cluster.fodcAgent.enabled 
}}
+        - name: fodc-agent
+          image: {{ $.Values.cluster.fodcAgent.image.repository }}:{{ default 
$.Values.image.tag $.Values.cluster.fodcAgent.image.tag }}
+          imagePullPolicy: {{ $.Values.cluster.fodcAgent.image.pullPolicy }}
+          {{- with $.Values.cluster.fodcAgent.containerSecurityContext }}
+          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

Review Comment:
   Perform a connection check in the agent. If I remember correctly, the 
agent's connection manager has the capacity to handle reconnections in case of 
failure.



##########
chart/values.yaml:
##########
@@ -734,6 +737,206 @@ cluster:
         ##
         failureThreshold: 60
 
+  ## @section Configuration for FODC (First Occurrence Data Collection) Proxy 
component
+  ##
+  fodcProxy:
+    ## @param cluster.fodcProxy.enabled Enable FODC Proxy deployment (boolean)
+    enabled: true
+    ## @param cluster.fodcProxy.podAnnotations Pod annotations for Proxy
+    podAnnotations: {}
+    ## @param cluster.fodcProxy.securityContext Security context for Proxy pods
+    securityContext: {}
+    ## @param cluster.fodcProxy.containerSecurityContext Container-level 
security context for Proxy
+    containerSecurityContext: {}
+    ## @param cluster.fodcProxy.env Environment variables for Proxy pods
+    env: []
+    ## @param cluster.fodcProxy.priorityClassName Priority class name for 
Proxy pods
+    priorityClassName: ""
+    ## Update strategy for Proxy pods
+    updateStrategy:
+      ## @param cluster.fodcProxy.updateStrategy.type Update strategy type for 
Proxy pods
+      type: RollingUpdate
+      rollingUpdate:
+        ## @param 
cluster.fodcProxy.updateStrategy.rollingUpdate.maxUnavailable Maximum 
unavailable pods during update
+        maxUnavailable: 1
+        ## @param cluster.fodcProxy.updateStrategy.rollingUpdate.maxSurge 
Maximum surge pods during update
+        maxSurge: 1
+    ## @param cluster.fodcProxy.podDisruptionBudget Pod disruption budget for 
Proxy
+    podDisruptionBudget: {}
+    ## @param cluster.fodcProxy.tolerations Tolerations for Proxy pods
+    tolerations: []
+    ## @param cluster.fodcProxy.nodeSelector Node selector for Proxy pods
+    nodeSelector: []
+    ## @param cluster.fodcProxy.affinity Affinity rules for Proxy pods
+    affinity: {}
+    ## @param cluster.fodcProxy.podAffinityPreset Pod affinity preset for Proxy
+    podAffinityPreset: ""
+    ## @param cluster.fodcProxy.podAntiAffinityPreset Pod anti-affinity preset 
for Proxy
+    podAntiAffinityPreset: soft

Review Comment:
   They could take place when there are more than 1 replica.



##########
chart/values.yaml:
##########
@@ -734,6 +737,206 @@ cluster:
         ##
         failureThreshold: 60
 
+  ## @section Configuration for FODC (First Occurrence Data Collection) Proxy 
component
+  ##
+  fodcProxy:
+    ## @param cluster.fodcProxy.enabled Enable FODC Proxy deployment (boolean)
+    enabled: true
+    ## @param cluster.fodcProxy.podAnnotations Pod annotations for Proxy
+    podAnnotations: {}
+    ## @param cluster.fodcProxy.securityContext Security context for Proxy pods
+    securityContext: {}
+    ## @param cluster.fodcProxy.containerSecurityContext Container-level 
security context for Proxy
+    containerSecurityContext: {}
+    ## @param cluster.fodcProxy.env Environment variables for Proxy pods
+    env: []
+    ## @param cluster.fodcProxy.priorityClassName Priority class name for 
Proxy pods
+    priorityClassName: ""
+    ## Update strategy for Proxy pods
+    updateStrategy:
+      ## @param cluster.fodcProxy.updateStrategy.type Update strategy type for 
Proxy pods
+      type: RollingUpdate
+      rollingUpdate:
+        ## @param 
cluster.fodcProxy.updateStrategy.rollingUpdate.maxUnavailable Maximum 
unavailable pods during update
+        maxUnavailable: 1
+        ## @param cluster.fodcProxy.updateStrategy.rollingUpdate.maxSurge 
Maximum surge pods during update
+        maxSurge: 1
+    ## @param cluster.fodcProxy.podDisruptionBudget Pod disruption budget for 
Proxy
+    podDisruptionBudget: {}
+    ## @param cluster.fodcProxy.tolerations Tolerations for Proxy pods
+    tolerations: []
+    ## @param cluster.fodcProxy.nodeSelector Node selector for Proxy pods
+    nodeSelector: []
+    ## @param cluster.fodcProxy.affinity Affinity rules for Proxy pods
+    affinity: {}
+    ## @param cluster.fodcProxy.podAffinityPreset Pod affinity preset for Proxy
+    podAffinityPreset: ""
+    ## @param cluster.fodcProxy.podAntiAffinityPreset Pod anti-affinity preset 
for Proxy
+    podAntiAffinityPreset: soft
+    ## Resource requests/limits for Proxy
+    resources:
+      ## @param cluster.fodcProxy.resources.requests Resource requests for 
Proxy pods
+      requests: {}
+      ## @param cluster.fodcProxy.resources.limits Resource limits for Proxy 
pods
+      limits: {}
+    image:
+      ## @param cluster.fodcProxy.image.repository Docker repository for FODC 
Proxy
+      repository: ghcr.io/apache/skywalking-banyandb-fodc-proxy
+      ## @param cluster.fodcProxy.image.tag Image tag/version for FODC Proxy 
(empty for latest)
+      tag: ""
+      ## @param cluster.fodcProxy.image.pullPolicy Image pull policy for FODC 
Proxy
+      pullPolicy: IfNotPresent
+    grpcSvc:
+      ## @param cluster.fodcProxy.grpcSvc.labels Labels for Proxy gRPC service
+      labels: {}
+      ## @param cluster.fodcProxy.grpcSvc.annotations Annotations for Proxy 
gRPC service
+      annotations: {}
+      ## @param cluster.fodcProxy.grpcSvc.port Port number for Proxy gRPC 
service (Agent connections)
+      port: 17912
+    httpSvc:
+      ## @param cluster.fodcProxy.httpSvc.labels Labels for Proxy HTTP service
+      labels: {}
+      ## @param cluster.fodcProxy.httpSvc.annotations Annotations for Proxy 
HTTP service
+      annotations: {}
+      ## @param cluster.fodcProxy.httpSvc.port Port number for Proxy HTTP 
service
+      port: 17913
+      ## @param cluster.fodcProxy.httpSvc.type Service type for Proxy HTTP 
service (ClusterIP, LoadBalancer, NodePort)
+      type: LoadBalancer
+      ## @param cluster.fodcProxy.httpSvc.externalIPs External IP addresses 
for Proxy HTTP service
+      externalIPs: []
+      ## @param cluster.fodcProxy.httpSvc.loadBalancerIP Load balancer IP for 
Proxy HTTP service
+      loadBalancerIP: null
+      ## @param cluster.fodcProxy.httpSvc.loadBalancerSourceRanges Allowed 
source ranges for Proxy HTTP service
+      loadBalancerSourceRanges: []
+    ingress:

Review Comment:
   Would you like to make the proxy accessible outside the cluster? If not, 
ingress is obsolete. @Fine0830 @wu-sheng 



##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -134,6 +134,25 @@ spec:
             {{- toYaml . | nindent 12 }}
           {{- end }}
         {{- end }}
+        {{- if and $.Values.cluster.enabled $.Values.cluster.fodcAgent.enabled 
$.Values.cluster.fodcProxy.enabled }}

Review Comment:
   I do not recommend blocking the node boot until the proxy is available. FODC 
is a monitoring system that should not affect the functionality of production 
components.



##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -289,6 +308,108 @@ spec:
             {{- end }}
             {{- end }}
           {{- end }}
+        {{- if and $.Values.cluster.enabled $.Values.cluster.fodcAgent.enabled 
}}
+        - name: fodc-agent
+          image: {{ $.Values.cluster.fodcAgent.image.repository }}:{{ default 
$.Values.image.tag $.Values.cluster.fodcAgent.image.tag }}
+          imagePullPolicy: {{ $.Values.cluster.fodcAgent.image.pullPolicy }}
+          {{- with $.Values.cluster.fodcAgent.containerSecurityContext }}
+          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.fodcProxy.grpcSvc.port }} \
+                --node-ip=${POD_NAME}.{{ template "banyandb.fullname" $ 
}}-data-{{ $roleName }}-headless.${POD_NAMESPACE} \

Review Comment:
   Isn't the node IP the localhost/127.0.0.1? Why do you use the headless host 
name?



##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -289,6 +308,108 @@ spec:
             {{- end }}
             {{- end }}
           {{- end }}
+        {{- if and $.Values.cluster.enabled $.Values.cluster.fodcAgent.enabled 
}}
+        - name: fodc-agent
+          image: {{ $.Values.cluster.fodcAgent.image.repository }}:{{ default 
$.Values.image.tag $.Values.cluster.fodcAgent.image.tag }}
+          imagePullPolicy: {{ $.Values.cluster.fodcAgent.image.pullPolicy }}
+          {{- with $.Values.cluster.fodcAgent.containerSecurityContext }}
+          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.fodcProxy.grpcSvc.port }} \
+                --node-ip=${POD_NAME}.{{ template "banyandb.fullname" $ 
}}-data-{{ $roleName }}-headless.${POD_NAMESPACE} \
+                --node-port={{ $.Values.cluster.data.nodeTemplate.grpcSvc.port 
}} \
+                --node-role=datanode-{{ $roleName }} \
+                --metrics-endpoint=http://127.0.0.1:2121/metrics \
+                --poll-metrics-interval={{ 
$.Values.cluster.fodcAgent.config.scrapeInterval }} \
+                --prometheus-listen-addr=:{{ 
$.Values.cluster.fodcAgent.httpPort }} \
+                --max-metrics-memory-usage-percentage={{ 
$.Values.cluster.fodcAgent.config.ktmEnabled | ternary 10 5 }} \
+                --heartbeat-interval={{ 
$.Values.cluster.fodcAgent.config.heartbeatInterval }} \
+                --reconnect-interval=10s

Review Comment:
   Could you please parameterize it?



##########
chart/values.yaml:
##########
@@ -734,6 +737,206 @@ cluster:
         ##
         failureThreshold: 60
 
+  ## @section Configuration for FODC (First Occurrence Data Collection) Proxy 
component
+  ##
+  fodcProxy:
+    ## @param cluster.fodcProxy.enabled Enable FODC Proxy deployment (boolean)
+    enabled: true
+    ## @param cluster.fodcProxy.podAnnotations Pod annotations for Proxy
+    podAnnotations: {}
+    ## @param cluster.fodcProxy.securityContext Security context for Proxy pods
+    securityContext: {}
+    ## @param cluster.fodcProxy.containerSecurityContext Container-level 
security context for Proxy
+    containerSecurityContext: {}
+    ## @param cluster.fodcProxy.env Environment variables for Proxy pods
+    env: []
+    ## @param cluster.fodcProxy.priorityClassName Priority class name for 
Proxy pods
+    priorityClassName: ""
+    ## Update strategy for Proxy pods
+    updateStrategy:

Review Comment:
   updateStrategy is useless since the replica is always 1. 



-- 
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]

Reply via email to