Copilot commented on code in PR #9292:
URL: https://github.com/apache/gravitino/pull/9292#discussion_r2580871208
##########
dev/charts/gravitino/templates/poddisruptionbudget.yaml:
##########
@@ -0,0 +1,46 @@
+{{- /*
+ 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.podDisruptionBudget.enabled }}
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+ name: {{ include "gravitino.fullname" . }}
+ namespace: {{ include "gravitino.namespace" . }}
+ labels:
+ {{- include "gravitino.labels" . | nindent 4 }}
+ {{- with .Values.podDisruptionBudget.labels }}
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+ {{- with .Values.podDisruptionBudget.annotations }}
+ annotations:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ selector:
+ matchLabels:
+ app: {{ include "gravitino.name" . }}
+ release: {{ .Release.Name }}
+ {{- if .Values.podDisruptionBudget.minAvailable }}
+ minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
+ {{- end }}
+ {{- if .Values.podDisruptionBudget.maxUnavailable }}
+ maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
+ {{- end }}
Review Comment:
The template allows both `minAvailable` and `maxUnavailable` to be specified
simultaneously, which can lead to overly restrictive PDB configurations. While
the documentation warns about this in the comments (line 602-603), consider
adding validation logic to prevent both from being set or to prefer one over
the other.
For example, you could add:
```yaml
{{- if and .Values.podDisruptionBudget.minAvailable
.Values.podDisruptionBudget.maxUnavailable }}
{{- fail "podDisruptionBudget.minAvailable and
podDisruptionBudget.maxUnavailable cannot both be specified. Please specify
only one." }}
{{- end }}
```
##########
dev/charts/gravitino-iceberg-rest-server/values.yaml:
##########
@@ -347,3 +347,67 @@ nodeSelector: {}
tolerations: []
affinity: {}
+
+## PodDisruptionBudget configuration
+## PodDisruptionBudgets limit the number of pods that can be down
simultaneously during voluntary disruptions
+## (such as node drains, cluster upgrades, or pod evictions), ensuring high
availability and service continuity.
+## ref: https://kubernetes.io/docs/tasks/run-application/configure-pdb/
+##
+podDisruptionBudget:
+ ## @param podDisruptionBudget.enabled Enable PodDisruptionBudget creation
+ ## Set to true to create a PodDisruptionBudget resource for the Iceberg REST
Server deployment
+ ##
+ enabled: false
+
+ ## @param podDisruptionBudget.minAvailable Minimum number/percentage of pods
that must remain available
+ ## Specify either an integer (e.g., 1, 2) or a percentage string (e.g.,
"50%")
+ ## This ensures at least this many pods stay running during voluntary
disruptions
+ ##
+ ## Examples:
+ ## minAvailable: 1 # At least 1 pod must remain available
+ ## minAvailable: 2 # At least 2 pods must remain available
+ ## minAvailable: "50%" # At least 50% of pods must remain available
+ ##
+ ## When to use minAvailable:
+ ## - Use when you want to guarantee a minimum number of pods stay running
+ ## - Recommended for production deployments to ensure service availability
+ ## - With single replica (replicas: 1), minAvailable: 1 prevents all
voluntary disruptions
+ ## - With multiple replicas, allows disruptions while maintaining minimum
availability
+ ##
+ minAvailable: 1
+
+ ## @param podDisruptionBudget.maxUnavailable Maximum number/percentage of
pods that can be unavailable
+ ## Specify either an integer (e.g., 1, 2) or a percentage string (e.g.,
"50%")
+ ## This limits how many pods can be down simultaneously during voluntary
disruptions
+ ##
+ ## Examples:
+ ## maxUnavailable: 1 # At most 1 pod can be unavailable
+ ## maxUnavailable: 2 # At most 2 pods can be unavailable
+ ## maxUnavailable: "25%" # At most 25% of pods can be unavailable
+ ##
+ ## When to use maxUnavailable:
+ ## - Use when you want to control the rate of disruptions
+ ## - Useful for rolling updates and gradual scaling operations
+ ## - More flexible than minAvailable when scaling up/down
+ ##
+ ## IMPORTANT: Specify either minAvailable OR maxUnavailable, not both
+ ## If both are specified, Kubernetes will use both constraints, which may be
overly restrictive
+ ##
+ maxUnavailable: ""
+
+ ## @param podDisruptionBudget.labels Additional labels to apply to the
PodDisruptionBudget resource
+ ## These labels are merged with the default Helm labels
+ ##
+ labels: {}
+ # custom-label: value
+
+ ## @param podDisruptionBudget.annotations Additional annotations to apply to
the PodDisruptionBudget resource
+ ## Useful for adding metadata or integration with other tools
+ ##
+ annotations: {}
+ # custom-annotation: value
+
+## Relationship between PDB and replica count:
+## - Single replica (replicas: 1) + minAvailable: 1 = No voluntary disruptions
allowed
+## - Multiple replicas (replicas: 2+) + minAvailable: 1 = Allows disruptions
while keeping at least 1 pod running
+## - Multiple replicas (replicas: 3+) + maxUnavailable: 1 = Allows 1 pod to be
disrupted at a time
Review Comment:
The documentation for the PodDisruptionBudget relationship with replica
count is incomplete compared to the gravitino chart
(dev/charts/gravitino/values.yaml lines 619-631). Consider adding the missing
best practices section and the additional explanation present in the gravitino
chart for consistency:
```yaml
## Relationship between PDB and replica count:
## - Single replica (replicas: 1) + minAvailable: 1 = No voluntary
disruptions allowed
## This prevents node drains and upgrades from evicting the only pod
## - Multiple replicas (replicas: 2+) + minAvailable: 1 = Allows disruptions
while keeping at least 1 pod running
## - Multiple replicas (replicas: 3+) + maxUnavailable: 1 = Allows 1 pod to
be disrupted at a time
##
## Best practices:
## - For production: Enable PDB with appropriate minAvailable or
maxUnavailable
## - For development/testing: Keep PDB disabled (default) for faster
iterations
## - Consider your replica count when setting PDB values
## - Test PDB behavior in staging before applying to production
```
```suggestion
## - Single replica (replicas: 1) + minAvailable: 1 = No voluntary
disruptions allowed
## This prevents node drains and upgrades from evicting the only pod
## - Multiple replicas (replicas: 2+) + minAvailable: 1 = Allows disruptions
while keeping at least 1 pod running
## - Multiple replicas (replicas: 3+) + maxUnavailable: 1 = Allows 1 pod to
be disrupted at a time
##
## Best practices:
## - For production: Enable PDB with appropriate minAvailable or
maxUnavailable
## - For development/testing: Keep PDB disabled (default) for faster
iterations
## - Consider your replica count when setting PDB values
## - Test PDB behavior in staging before applying to production
```
##########
dev/charts/gravitino-iceberg-rest-server/templates/poddisruptionbudget.yaml:
##########
@@ -0,0 +1,46 @@
+{{- /*
+ 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.podDisruptionBudget.enabled }}
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+ name: {{ include "gravitino-iceberg-rest-server.fullname" . }}
+ namespace: {{ include "gravitino-iceberg-rest-server.namespace" . }}
+ labels:
+ {{- include "gravitino-iceberg-rest-server.labels" . | nindent 4 }}
+ {{- with .Values.podDisruptionBudget.labels }}
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+ {{- with .Values.podDisruptionBudget.annotations }}
+ annotations:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ selector:
+ matchLabels:
+ app: {{ include "gravitino-iceberg-rest-server.name" . }}
+ release: {{ .Release.Name }}
+ {{- if .Values.podDisruptionBudget.minAvailable }}
+ minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
+ {{- end }}
+ {{- if .Values.podDisruptionBudget.maxUnavailable }}
+ maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
+ {{- end }}
Review Comment:
The template allows both `minAvailable` and `maxUnavailable` to be specified
simultaneously, which can lead to overly restrictive PDB configurations. While
the documentation warns about this in the comments (line 393-394 in
values.yaml), consider adding validation logic to prevent both from being set
or to prefer one over the other.
For example, you could add:
```yaml
{{- if and .Values.podDisruptionBudget.minAvailable
.Values.podDisruptionBudget.maxUnavailable }}
{{- fail "podDisruptionBudget.minAvailable and
podDisruptionBudget.maxUnavailable cannot both be specified. Please specify
only one." }}
{{- end }}
```
--
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]
