Copilot commented on code in PR #9292:
URL: https://github.com/apache/gravitino/pull/9292#discussion_r2587755218
##########
dev/charts/gravitino-iceberg-rest-server/templates/poddisruptionbudget.yaml:
##########
@@ -0,0 +1,51 @@
+{{- /*
+ 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 }}
+{{- $minAvailableSet := and (ne (toString
.Values.podDisruptionBudget.minAvailable) "") (or
.Values.podDisruptionBudget.minAvailable (eq
(.Values.podDisruptionBudget.minAvailable | int) 0)) }}
+{{- $maxUnavailableSet := ne (toString
.Values.podDisruptionBudget.maxUnavailable) "" }}
+{{- if and $minAvailableSet $maxUnavailableSet }}
+{{- fail "podDisruptionBudget.minAvailable and
podDisruptionBudget.maxUnavailable cannot both be specified. Please specify
only one." }}
+{{- end }}
Review Comment:
Missing validation: When PodDisruptionBudget is enabled, at least one of
`minAvailable` or `maxUnavailable` must be specified. The current code only
validates that both aren't set simultaneously (line 23-25), but doesn't check
if neither is set. According to Kubernetes API requirements, a PDB must have at
least one of these fields defined. Add validation to fail if both fields are
empty when enabled.
```suggestion
{{- end }}
{{- if not (or $minAvailableSet $maxUnavailableSet) }}
{{- fail "podDisruptionBudget.enabled is true, but neither
podDisruptionBudget.minAvailable nor podDisruptionBudget.maxUnavailable is
specified. Please specify one." }}
{{- end }}
```
##########
dev/charts/gravitino/values.yaml:
##########
@@ -556,3 +556,76 @@ tolerations: []
## ref:
https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity
##
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 Gravitino
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
Review Comment:
[nitpick] The default value `minAvailable: 1` with `enabled: false` may be
confusing. When PDB is disabled, this value is not used, but it appears as if
it's the active configuration. Consider adding a comment clarifying that this
is only used when `enabled: true`, or use an empty string as the default to
make it clear users need to explicitly set it when enabling PDB.
```suggestion
# Only used when podDisruptionBudget.enabled: true. Set explicitly when
enabling PDB.
minAvailable: ""
```
##########
dev/charts/gravitino-iceberg-rest-server/values.yaml:
##########
@@ -347,3 +347,76 @@ 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
Review Comment:
Inaccurate documentation: The comment states "If both are specified,
Kubernetes will use both constraints, which may be overly restrictive".
However, according to Kubernetes documentation, specifying both `minAvailable`
and `maxUnavailable` is invalid and will cause the PDB creation to fail with an
API validation error. The template correctly validates and fails when both are
set (lines 23-25 in poddisruptionbudget.yaml), so this comment should be
updated to reflect that both cannot be specified, not that they can be but
would be overly restrictive.
```suggestion
## If both are specified, Kubernetes will reject the PodDisruptionBudget
and fail with an API validation error
```
##########
dev/charts/gravitino-iceberg-rest-server/values.yaml:
##########
@@ -347,3 +347,76 @@ 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
+ ##
Review Comment:
[nitpick] The default value `minAvailable: 1` with `enabled: false` may be
confusing. When PDB is disabled, this value is not used, but it appears as if
it's the active configuration. Consider adding a comment clarifying that this
is only used when `enabled: true`, or use an empty string as the default to
make it clear users need to explicitly set it when enabling PDB.
```suggestion
##
# Only used when podDisruptionBudget.enabled is true
```
##########
dev/charts/gravitino/values.yaml:
##########
@@ -556,3 +556,76 @@ tolerations: []
## ref:
https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity
##
affinity: {}
+
+## PodDisruptionBudget configuration
Review Comment:
Spelling error in comment: "disrutption" should be "disruption".
##########
dev/charts/gravitino-iceberg-rest-server/values.yaml:
##########
@@ -347,3 +347,76 @@ nodeSelector: {}
tolerations: []
affinity: {}
+
+## PodDisruptionBudget configuration
Review Comment:
Spelling error in comment: "disrutption" should be "disruption".
##########
dev/charts/gravitino/templates/poddisruptionbudget.yaml:
##########
@@ -0,0 +1,51 @@
+{{- /*
+ 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 }}
+{{- $minAvailableSet := and (ne (toString
.Values.podDisruptionBudget.minAvailable) "") (or
.Values.podDisruptionBudget.minAvailable (eq
(.Values.podDisruptionBudget.minAvailable | int) 0)) }}
+{{- $maxUnavailableSet := ne (toString
.Values.podDisruptionBudget.maxUnavailable) "" }}
+{{- if and $minAvailableSet $maxUnavailableSet }}
+{{- fail "podDisruptionBudget.minAvailable and
podDisruptionBudget.maxUnavailable cannot both be specified. Please specify
only one." }}
+{{- end }}
Review Comment:
Missing validation: When PodDisruptionBudget is enabled, at least one of
`minAvailable` or `maxUnavailable` must be specified. The current code only
validates that both aren't set simultaneously (line 23-25), but doesn't check
if neither is set. According to Kubernetes API requirements, a PDB must have at
least one of these fields defined. Add validation to fail if both fields are
empty when enabled.
```suggestion
{{- end }}
{{- if not (or $minAvailableSet $maxUnavailableSet) }}
{{- fail "podDisruptionBudget.enabled is true, but neither
podDisruptionBudget.minAvailable nor podDisruptionBudget.maxUnavailable is
specified. Please specify at least one." }}
{{- end }}
```
##########
dev/charts/gravitino/values.yaml:
##########
@@ -556,3 +556,76 @@ tolerations: []
## ref:
https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity
##
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 Gravitino
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
Review Comment:
Inaccurate documentation: The comment states "If both are specified,
Kubernetes will use both constraints, which may be overly restrictive".
However, according to Kubernetes documentation, specifying both `minAvailable`
and `maxUnavailable` is invalid and will cause the PDB creation to fail with an
API validation error. The template correctly validates and fails when both are
set (lines 23-25 in poddisruptionbudget.yaml), so this comment should be
updated to reflect that both cannot be specified, not that they can be but
would be overly restrictive.
```suggestion
## If both are specified, Kubernetes will reject the PodDisruptionBudget
and fail with an API validation error
```
##########
dev/charts/gravitino-iceberg-rest-server/templates/poddisruptionbudget.yaml:
##########
@@ -0,0 +1,51 @@
+{{- /*
+ 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 }}
+{{- $minAvailableSet := and (ne (toString
.Values.podDisruptionBudget.minAvailable) "") (or
.Values.podDisruptionBudget.minAvailable (eq
(.Values.podDisruptionBudget.minAvailable | int) 0)) }}
+{{- $maxUnavailableSet := ne (toString
.Values.podDisruptionBudget.maxUnavailable) "" }}
+{{- if and $minAvailableSet $maxUnavailableSet }}
+{{- fail "podDisruptionBudget.minAvailable and
podDisruptionBudget.maxUnavailable cannot both be specified. Please specify
only one." }}
+{{- end }}
+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 }}
Review Comment:
The conditional logic for rendering `minAvailable` and `maxUnavailable` is
inconsistent with the validation logic. Lines 21-22 set `$minAvailableSet` and
`$maxUnavailableSet` to validate that both aren't set, but lines 45-50 use
simpler conditionals that only check if the values are truthy. This means if
`minAvailable: 0` is set (a valid value), the validation would pass but line 45
would not render it because `0` is falsy in Helm templates. Consider using the
same logic for rendering: `{{- if $minAvailableSet }}` instead of `{{- if
.Values.podDisruptionBudget.minAvailable }}`.
```suggestion
{{- if $minAvailableSet }}
minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
{{- end }}
{{- if $maxUnavailableSet }}
```
##########
dev/charts/gravitino/templates/poddisruptionbudget.yaml:
##########
@@ -0,0 +1,51 @@
+{{- /*
+ 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 }}
+{{- $minAvailableSet := and (ne (toString
.Values.podDisruptionBudget.minAvailable) "") (or
.Values.podDisruptionBudget.minAvailable (eq
(.Values.podDisruptionBudget.minAvailable | int) 0)) }}
+{{- $maxUnavailableSet := ne (toString
.Values.podDisruptionBudget.maxUnavailable) "" }}
+{{- if and $minAvailableSet $maxUnavailableSet }}
+{{- fail "podDisruptionBudget.minAvailable and
podDisruptionBudget.maxUnavailable cannot both be specified. Please specify
only one." }}
+{{- end }}
+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 }}
Review Comment:
The conditional logic for rendering `minAvailable` and `maxUnavailable` is
inconsistent with the validation logic. Lines 21-22 set `$minAvailableSet` and
`$maxUnavailableSet` to validate that both aren't set, but lines 45-50 use
simpler conditionals that only check if the values are truthy. This means if
`minAvailable: 0` is set (a valid value), the validation would pass but line 45
would not render it because `0` is falsy in Helm templates. Consider using the
same logic for rendering: `{{- if $minAvailableSet }}` instead of `{{- if
.Values.podDisruptionBudget.minAvailable }}`.
```suggestion
{{- if $minAvailableSet }}
minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
{{- end }}
{{- if $maxUnavailableSet }}
```
--
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]
