Copilot commented on code in PR #17946: URL: https://github.com/apache/pinot/pull/17946#discussion_r2977591953
########## helm/pinot/templates/zookeeper/statefulset.yaml: ########## @@ -0,0 +1,195 @@ +# +# 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. +# + +{{- if .Values.zookeeper.enabled }} +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: {{ include "pinot.zookeeper.fullname" . }} + namespace: {{ include "pinot.namespace" . }} + labels: + {{- include "pinot.zookeeperLabels" . | nindent 4 }} +spec: + selector: + matchLabels: + {{- include "pinot.zookeeperMatchLabels" . | nindent 6 }} + serviceName: {{ include "pinot.zookeeper.headless" . }} + replicas: {{ .Values.zookeeper.replicaCount }} + updateStrategy: + type: RollingUpdate + podManagementPolicy: Parallel + template: + metadata: + labels: + {{- include "pinot.zookeeperLabels" . | nindent 8 }} + {{- with .Values.zookeeper.podAnnotations }} + annotations: + {{- toYaml . | nindent 8 }} + {{- end }} + spec: + {{- with .Values.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.securityContext }} + securityContext: + {{- toYaml . | nindent 8 }} + {{- end }} + containers: + - name: zookeeper + image: "{{ .Values.zookeeper.image.repository }}:{{ .Values.zookeeper.image.tag }}" + imagePullPolicy: {{ .Values.zookeeper.image.pullPolicy }} + env: + - name: ZOO_MY_ID + valueFrom: + fieldRef: + fieldPath: metadata.labels['apps.kubernetes.io/pod-index'] Review Comment: `ZOO_MY_ID` is being populated from the `apps.kubernetes.io/pod-index` label, but the container command later recomputes and exports `ZOO_MY_ID` from the pod hostname ordinal. This makes the fieldRef redundant and also relies on a label that may not exist in all clusters. Consider removing the fieldRef-based `ZOO_MY_ID` env entry (and updating the adjacent comment) to avoid confusion and ensure the ID source is consistent. ```suggestion ``` ########## helm/pinot/templates/zookeeper/statefulset.yaml: ########## @@ -0,0 +1,195 @@ +# +# 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. +# + +{{- if .Values.zookeeper.enabled }} +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: {{ include "pinot.zookeeper.fullname" . }} + namespace: {{ include "pinot.namespace" . }} + labels: + {{- include "pinot.zookeeperLabels" . | nindent 4 }} +spec: + selector: + matchLabels: + {{- include "pinot.zookeeperMatchLabels" . | nindent 6 }} + serviceName: {{ include "pinot.zookeeper.headless" . }} + replicas: {{ .Values.zookeeper.replicaCount }} + updateStrategy: + type: RollingUpdate + podManagementPolicy: Parallel + template: + metadata: + labels: + {{- include "pinot.zookeeperLabels" . | nindent 8 }} + {{- with .Values.zookeeper.podAnnotations }} + annotations: + {{- toYaml . | nindent 8 }} + {{- end }} + spec: + {{- with .Values.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.zookeeper.securityContext }} + securityContext: + {{- toYaml . | nindent 8 }} + {{- end }} + containers: + - name: zookeeper + image: "{{ .Values.zookeeper.image.repository }}:{{ .Values.zookeeper.image.tag }}" + imagePullPolicy: {{ .Values.zookeeper.image.pullPolicy }} + env: + - name: ZOO_MY_ID + valueFrom: + fieldRef: + fieldPath: metadata.labels['apps.kubernetes.io/pod-index'] + - name: ZOO_SERVERS + value: {{ include "pinot.zookeeper.servers" . | quote }} + - name: ZOO_TICK_TIME + value: {{ .Values.zookeeper.tickTime | default 2000 | quote }} + - name: ZOO_INIT_LIMIT + value: {{ .Values.zookeeper.initLimit | default 10 | quote }} + - name: ZOO_SYNC_LIMIT + value: {{ .Values.zookeeper.syncLimit | default 5 | quote }} + - name: ZOO_MAX_CLIENT_CNXNS + value: {{ .Values.zookeeper.maxClientCnxns | default 60 | quote }} + - name: ZOO_AUTOPURGE_PURGE_INTERVAL + value: {{ .Values.zookeeper.autopurge.purgeInterval | default 1 | quote }} + - name: ZOO_AUTOPURGE_SNAP_RETAIN_COUNT + value: {{ .Values.zookeeper.autopurge.snapRetainCount | default 5 | quote }} + - name: JVMFLAGS + value: "-Xmx{{ .Values.zookeeper.heapSize }}m -Xms{{ .Values.zookeeper.heapSize }}m {{ .Values.zookeeper.jvmFlags }}" + - name: ZOO_4LW_COMMANDS_WHITELIST + value: {{ .Values.zookeeper.fourLetterWordsWhitelist | default "srvr,mntr,ruok,conf" | quote }} + - name: ZOO_ADMINSERVER_ENABLED + value: {{ .Values.zookeeper.adminServer.enabled | default false | quote }} + {{- with .Values.zookeeper.extraEnv }} + {{- toYaml . | nindent 10 }} + {{- end }} + command: + - bash + - -c + - | + # The official ZooKeeper image uses 1-based IDs, but pod-index is 0-based + HOST=$(hostname -s) + if [[ $HOST =~ (.*)-([0-9]+)$ ]]; then + export ZOO_MY_ID=$(( ${BASH_REMATCH[2]} + 1 )) + else + echo "Failed to extract ordinal from hostname $HOST" + exit 1 + fi + exec /docker-entrypoint.sh zkServer.sh start-foreground + ports: + - name: client + containerPort: {{ .Values.zookeeper.port }} + - name: follower + containerPort: 2888 + - name: election + containerPort: 3888 + {{- if .Values.zookeeper.adminServer.enabled }} + - name: admin + containerPort: {{ .Values.zookeeper.adminServer.port | default 8080 }} Review Comment: `zookeeper.adminServer.port` is only used to set the containerPort, but the template doesn’t pass any configuration/env var to make ZooKeeper actually listen on that port. As-is, changing the value may expose a port that the process isn’t bound to. Either wire this value into ZooKeeper’s AdminServer configuration (so the container listens on the same port) or drop the `port` setting to avoid a misleading knob. ```suggestion containerPort: 8080 ``` ########## helm/pinot/UPGRADING.md: ########## @@ -0,0 +1,136 @@ +<!-- + + 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. + +--> + +# Upgrading the Pinot Helm Chart + +## From 0.x to 1.0.0 + +Version 1.0.0 replaces the Bitnami ZooKeeper subchart with native Helm +templates using the [official Apache ZooKeeper Docker image](https://hub.docker.com/_/zookeeper). +This is a **breaking change** that requires manual intervention when upgrading +existing deployments. + +### Why is this breaking? + +1. **Immutable StatefulSet selectors**: The Bitnami chart uses + `app.kubernetes.io/name: zookeeper` labels, while the new templates use + `app: pinot, component: zookeeper`. StatefulSet selectors cannot be changed + in-place, so `helm upgrade` will fail. + +2. **Different data paths**: The Bitnami image stores data at + `/bitnami/zookeeper`, while the official image uses `/data`. Even though the + PVC name (`data`) is the same, the new container will not find existing data. + +3. **Removed Bitnami-specific values**: Options like `zookeeper.image.registry`, + `zookeeper.global.security.allowInsecureImages`, `zookeeper.tls.*`, and + `zookeeper.auth.*` no longer apply. + +### Migration steps + +> **Important**: ZooKeeper stores Pinot cluster metadata (table configs, schemas, +> segment assignments). Losing this data means the Pinot cluster will need to be +> reconfigured. Plan accordingly. + +#### Option A: Fresh ZooKeeper (simplest, requires Pinot reconfiguration) + +```bash +NAMESPACE=pinot-quickstart +RELEASE=pinot + +# 1. Delete the old ZooKeeper StatefulSet (pods will be terminated) +kubectl delete statefulset ${RELEASE}-zookeeper -n ${NAMESPACE} + +# 2. Delete old ZooKeeper PVCs +kubectl delete pvc -l app.kubernetes.io/name=zookeeper -n ${NAMESPACE} + +# 3. Upgrade the Helm release +helm upgrade ${RELEASE} -n ${NAMESPACE} ./helm/pinot + +# 4. Recreate your Pinot tables and schemas +``` + +#### Option B: Migrate ZooKeeper data (best-effort metadata preservation) + +> **Note**: This option copies the ZooKeeper data directory from the old +> Bitnami mount path to the new official image mount path. This preserves +> Pinot cluster metadata (table configs, schemas, segment assignments) on a +> best-effort basis. Verify your cluster state after migration. + +```bash +NAMESPACE=pinot-quickstart +RELEASE=pinot + +# 1. Copy data from the Bitnami path to the official image path within +# the existing PVC, while the old ZooKeeper is still running. +kubectl exec -n ${NAMESPACE} ${RELEASE}-zookeeper-0 -- \ + bash -c 'cp -a /bitnami/zookeeper/data/* /tmp/ 2>/dev/null; echo "Data backed up to /tmp"' + +# 2. Delete the old ZooKeeper StatefulSet and pods. +kubectl delete statefulset ${RELEASE}-zookeeper -n ${NAMESPACE} + +# 3. Delete old PVCs (mount paths are incompatible between images). +kubectl delete pvc -l app.kubernetes.io/name=zookeeper -n ${NAMESPACE} Review Comment: In Option B, step 1 copies ZooKeeper data to /tmp, but steps 2–3 delete the StatefulSet and PVCs. That sequence can’t preserve ZooKeeper metadata (the /tmp data is lost when the pod is deleted, and PVC deletion discards the data entirely). Please update the migration instructions to either (a) copy data to a durable location outside the pod/PVC before deletion, or (b) reuse the existing PVC and copy data into the new image’s expected paths (/data and /datalog) before switching images. ########## helm/pinot/values.yaml: ########## @@ -820,3 +851,18 @@ zookeeper: # labelSelector: # matchLabels: # release: zookeeper + + ## Node selector for ZooKeeper pods + nodeSelector: {} + + ## Tolerations for ZooKeeper pods + tolerations: [] + + ## Pod annotations + podAnnotations: {} + + ## Pod security context + securityContext: {} + Review Comment: The new `zookeeper.securityContext` value is applied at the *pod* level in the StatefulSet. Elsewhere in this chart, pod-level and container-level contexts are separated (`podSecurityContext` vs `securityContext`, e.g. controller/server/minion sections in values.yaml). Consider renaming this to `zookeeper.podSecurityContext` (and optionally adding a separate `zookeeper.containerSecurityContext`) to keep semantics consistent and avoid users assuming this is a container securityContext. ```suggestion podSecurityContext: {} ## Container security context containerSecurityContext: {} ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
