Copilot commented on code in PR #11968:
URL: https://github.com/apache/cloudstack/pull/11968#discussion_r3969663592
##########
plugins/integrations/kubernetes-service/src/main/resources/script/delete-pv-reclaimpolicy-delete:
##########
@@ -33,23 +33,33 @@ delete_workloads_using_pvc() {
local deleted_count=0
# Find & delete any deployment using the PVC
- /opt/bin/kubectl get deployments -n "$namespace" -o json 2>/dev/null |
grep -l "$pvc_name" | \
- while IFS= read -r deployment; do
- if [ -n "$deployment" ]; then
- deployment_name=$(echo "$deployment" | cut -d'/' -f2)
- echo "$(timestamp) - Deleting Deployment: $deployment_name"
- /opt/bin/kubectl delete deployment "$deployment_name" -n
"$namespace" --ignore-not-found=true
+ # Iterate resource names and check the volumes via jsonpath to avoid grep
-l on JSON (which prints "(standard input)")
+ /opt/bin/kubectl get deployments -n "$namespace" -o name 2>/dev/null | \
+ while IFS= read -r resource; do
+ if [ -z "$resource" ]; then
+ continue
+ fi
+ name=${resource#*/}
+ dep_volumes=$(/opt/bin/kubectl get deployment "$name" -n "$namespace"
-o jsonpath='{.spec.template.spec.volumes[*].persistentVolumeClaim.claimName}'
2>/dev/null || echo "")
+ if [ -n "$dep_volumes" ] && echo "$dep_volumes" | grep -F -q
"$pvc_name"; then
+ echo "$(timestamp) - Deleting Deployment: $name"
+ /opt/bin/kubectl delete deployment "$name" -n "$namespace"
--ignore-not-found=true
deleted_count=$((deleted_count + 1))
fi
done
# Find and delete any StatefulSet using the PVC
- /opt/bin/kubectl get statefulsets -n "$namespace" -o json 2>/dev/null |
grep -l "$pvc_name" | \
- while IFS= read -r sts; do
- if [ -n "$sts" ]; then
- sts_name=$(echo "$sts" | cut -d'/' -f2)
- echo "$(timestamp) - Deleting StatefulSet: $sts_name"
- /opt/bin/kubectl delete statefulset "$sts_name" -n "$namespace"
--ignore-not-found=true
+ /opt/bin/kubectl get statefulsets -n "$namespace" -o name 2>/dev/null | \
+ while IFS= read -r resource; do
+ if [ -z "$resource" ]; then
+ continue
+ fi
+ name=${resource#*/}
+ # Check both template volumes and volumeClaimTemplates
+ sts_volumes=$(/opt/bin/kubectl get statefulset "$name" -n "$namespace"
-o jsonpath='{.spec.template.spec.volumes[*].persistentVolumeClaim.claimName}
{.spec.volumeClaimTemplates[*].metadata.name}' 2>/dev/null || echo "")
Review Comment:
The `volumeClaimTemplates[*].metadata.name` values are template base names,
not the actual PVC names created by a StatefulSet (which are typically
`<templateName>-<statefulsetName>-<ordinal>`). As written, this will usually
fail to detect the owning StatefulSet for a real PVC like `data-mysql-0`, and
it can also create false positives for PVCs whose name happens to equal a
template name. A more reliable approach is to fetch the PVC and use
`metadata.ownerReferences` (or, if that’s not viable, parse the expected
generated name format using the StatefulSet name and template names).
##########
plugins/integrations/kubernetes-service/src/main/resources/script/delete-pv-reclaimpolicy-delete:
##########
@@ -33,23 +33,33 @@ delete_workloads_using_pvc() {
local deleted_count=0
# Find & delete any deployment using the PVC
- /opt/bin/kubectl get deployments -n "$namespace" -o json 2>/dev/null |
grep -l "$pvc_name" | \
- while IFS= read -r deployment; do
- if [ -n "$deployment" ]; then
- deployment_name=$(echo "$deployment" | cut -d'/' -f2)
- echo "$(timestamp) - Deleting Deployment: $deployment_name"
- /opt/bin/kubectl delete deployment "$deployment_name" -n
"$namespace" --ignore-not-found=true
+ # Iterate resource names and check the volumes via jsonpath to avoid grep
-l on JSON (which prints "(standard input)")
+ /opt/bin/kubectl get deployments -n "$namespace" -o name 2>/dev/null | \
+ while IFS= read -r resource; do
+ if [ -z "$resource" ]; then
+ continue
+ fi
+ name=${resource#*/}
+ dep_volumes=$(/opt/bin/kubectl get deployment "$name" -n "$namespace"
-o jsonpath='{.spec.template.spec.volumes[*].persistentVolumeClaim.claimName}'
2>/dev/null || echo "")
+ if [ -n "$dep_volumes" ] && echo "$dep_volumes" | grep -F -q
"$pvc_name"; then
+ echo "$(timestamp) - Deleting Deployment: $name"
+ /opt/bin/kubectl delete deployment "$name" -n "$namespace"
--ignore-not-found=true
Review Comment:
This introduces an N+1 pattern (one `kubectl get deployment ...` per
deployment, similarly for other kinds), which can be very slow and puts extra
load on the API server in namespaces with many resources. Prefer fetching each
kind once (e.g., one `kubectl get <kind> -o json` / jsonpath “range” output)
and filtering locally, or derive the owning workload directly from the PVC (via
`metadata.ownerReferences`) to avoid scanning the entire namespace.
##########
plugins/integrations/kubernetes-service/src/main/resources/script/delete-pv-reclaimpolicy-delete:
##########
@@ -33,23 +33,33 @@ delete_workloads_using_pvc() {
local deleted_count=0
# Find & delete any deployment using the PVC
- /opt/bin/kubectl get deployments -n "$namespace" -o json 2>/dev/null |
grep -l "$pvc_name" | \
- while IFS= read -r deployment; do
- if [ -n "$deployment" ]; then
- deployment_name=$(echo "$deployment" | cut -d'/' -f2)
- echo "$(timestamp) - Deleting Deployment: $deployment_name"
- /opt/bin/kubectl delete deployment "$deployment_name" -n
"$namespace" --ignore-not-found=true
+ # Iterate resource names and check the volumes via jsonpath to avoid grep
-l on JSON (which prints "(standard input)")
+ /opt/bin/kubectl get deployments -n "$namespace" -o name 2>/dev/null | \
+ while IFS= read -r resource; do
+ if [ -z "$resource" ]; then
+ continue
+ fi
+ name=${resource#*/}
+ dep_volumes=$(/opt/bin/kubectl get deployment "$name" -n "$namespace"
-o jsonpath='{.spec.template.spec.volumes[*].persistentVolumeClaim.claimName}'
2>/dev/null || echo "")
+ if [ -n "$dep_volumes" ] && echo "$dep_volumes" | grep -F -q
"$pvc_name"; then
Review Comment:
The PVC match is substring-based. If `$pvc_name` is `data`, this will also
match `data2`, `my-data`, etc., potentially deleting workloads that don’t
actually reference the target PVC. Consider comparing exact tokens (e.g.,
normalize to one claim name per line and use an exact match) rather than
substring searching.
##########
plugins/integrations/kubernetes-service/src/main/resources/script/delete-pv-reclaimpolicy-delete:
##########
@@ -33,23 +33,33 @@ delete_workloads_using_pvc() {
local deleted_count=0
# Find & delete any deployment using the PVC
- /opt/bin/kubectl get deployments -n "$namespace" -o json 2>/dev/null |
grep -l "$pvc_name" | \
- while IFS= read -r deployment; do
- if [ -n "$deployment" ]; then
- deployment_name=$(echo "$deployment" | cut -d'/' -f2)
- echo "$(timestamp) - Deleting Deployment: $deployment_name"
- /opt/bin/kubectl delete deployment "$deployment_name" -n
"$namespace" --ignore-not-found=true
+ # Iterate resource names and check the volumes via jsonpath to avoid grep
-l on JSON (which prints "(standard input)")
+ /opt/bin/kubectl get deployments -n "$namespace" -o name 2>/dev/null | \
+ while IFS= read -r resource; do
Review Comment:
Because the `while` loop is fed by a pipeline, it will run in a subshell in
many shells, so `deleted_count` increments may not be visible after the loop
finishes. If `deleted_count` is used later in this function, switch to a
construct that avoids the subshell (e.g., redirect from process substitution or
a temporary file) so the counter is updated in the parent shell.
##########
plugins/integrations/kubernetes-service/src/main/resources/script/delete-pv-reclaimpolicy-delete:
##########
@@ -33,23 +33,33 @@ delete_workloads_using_pvc() {
local deleted_count=0
# Find & delete any deployment using the PVC
- /opt/bin/kubectl get deployments -n "$namespace" -o json 2>/dev/null |
grep -l "$pvc_name" | \
- while IFS= read -r deployment; do
- if [ -n "$deployment" ]; then
- deployment_name=$(echo "$deployment" | cut -d'/' -f2)
- echo "$(timestamp) - Deleting Deployment: $deployment_name"
- /opt/bin/kubectl delete deployment "$deployment_name" -n
"$namespace" --ignore-not-found=true
+ # Iterate resource names and check the volumes via jsonpath to avoid grep
-l on JSON (which prints "(standard input)")
+ /opt/bin/kubectl get deployments -n "$namespace" -o name 2>/dev/null | \
+ while IFS= read -r resource; do
+ if [ -z "$resource" ]; then
+ continue
+ fi
+ name=${resource#*/}
+ dep_volumes=$(/opt/bin/kubectl get deployment "$name" -n "$namespace"
-o jsonpath='{.spec.template.spec.volumes[*].persistentVolumeClaim.claimName}'
2>/dev/null || echo "")
+ if [ -n "$dep_volumes" ] && echo "$dep_volumes" | grep -F -q
"$pvc_name"; then
+ echo "$(timestamp) - Deleting Deployment: $name"
+ /opt/bin/kubectl delete deployment "$name" -n "$namespace"
--ignore-not-found=true
deleted_count=$((deleted_count + 1))
fi
done
Review Comment:
Because the `while` loop is fed by a pipeline, it will run in a subshell in
many shells, so `deleted_count` increments may not be visible after the loop
finishes. If `deleted_count` is used later in this function, switch to a
construct that avoids the subshell (e.g., redirect from process substitution or
a temporary file) so the counter is updated in the parent shell.
--
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]
