Copilot commented on code in PR #17946: URL: https://github.com/apache/pinot/pull/17946#discussion_r2977924887
########## 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 }} Review Comment: In this StatefulSet, the pod spec doesn’t set `terminationGracePeriodSeconds` or `serviceAccountName`, while the other Pinot workloads do (e.g. `/helm/pinot/templates/broker/statefulset.yaml:47-50`). This makes ZooKeeper behavior/configuration inconsistent across the chart (and prevents users from applying the chart-wide termination grace period and service account to ZooKeeper). Consider adding `terminationGracePeriodSeconds: {{ .Values.terminationGracePeriodSeconds }}` and `serviceAccountName: {{ include "pinot.serviceAccountName" . }}` here for consistency. ```suggestion {{- end }} serviceAccountName: {{ include "pinot.serviceAccountName" . }} terminationGracePeriodSeconds: {{ .Values.terminationGracePeriodSeconds }} ``` ########## helm/pinot/UPGRADING.md: ########## @@ -0,0 +1,141 @@ +<!-- + + 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} Review Comment: These `kubectl delete pvc` commands select PVCs only by `app.kubernetes.io/name=zookeeper`, which can match PVCs from other ZooKeeper releases in the same namespace and lead to accidental data loss. It’s safer to scope the selector to the Helm release as well (e.g. include `app.kubernetes.io/instance=${RELEASE}`) or explicitly list the PVC names. ```suggestion # 2. Delete old ZooKeeper PVCs for this Helm release only kubectl delete pvc -l app.kubernetes.io/name=zookeeper,app.kubernetes.io/instance=${RELEASE} -n ${NAMESPACE} ``` ########## helm/pinot/UPGRADING.md: ########## @@ -0,0 +1,141 @@ +<!-- + + 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 (preserves Pinot metadata) + +> **Note**: This option copies ZooKeeper data from the old Bitnami mount +> path (`/bitnami/zookeeper`) to a local backup, then restores it into +> the new official image's data directory (`/data`) after upgrade. This +> preserves Pinot cluster metadata (table configs, schemas, segment +> assignments). Verify your cluster state after migration. + +```bash +NAMESPACE=pinot-quickstart +RELEASE=pinot + +# 1. Back up ZooKeeper data to your local machine while old cluster is running. +kubectl cp ${NAMESPACE}/${RELEASE}-zookeeper-0:/bitnami/zookeeper/data ./zk-data-backup + +# 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} + +# 4. Upgrade the Helm release (creates new StatefulSet + PVCs). +helm upgrade ${RELEASE} -n ${NAMESPACE} ./helm/pinot + +# 5. Wait for the new ZooKeeper to be ready. +kubectl rollout status statefulset/${RELEASE}-zookeeper -n ${NAMESPACE} + +# 6. Stop ZooKeeper, restore data, then restart. +kubectl exec -n ${NAMESPACE} ${RELEASE}-zookeeper-0 -- \ + bash -c 'zkServer.sh stop' 2>/dev/null || true +kubectl cp ./zk-data-backup ${NAMESPACE}/${RELEASE}-zookeeper-0:/data Review Comment: `kubectl cp ./zk-data-backup ...:/data` will typically create `/data/zk-data-backup/...` (because `/data` already exists), rather than copying the *contents* into `/data`. That can prevent ZooKeeper from seeing the restored snapshot/logs. Consider using a trailing `/.` (copy contents) or copying into the exact expected data dir path so the restored files land directly under `/data`. ```suggestion kubectl cp ./zk-data-backup/. ${NAMESPACE}/${RELEASE}-zookeeper-0:/data ``` -- 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]
