fix: add pvc clean after resource delete #31
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements automatic cleanup of PersistentVolumeClaims (PVCs) when ApplicationDefinition resources are deleted or emptied. The changes address a common Kubernetes pattern where StatefulSet PVCs are not automatically garbage collected with their parent resources, requiring explicit cleanup logic.
Key changes:
- Added PVC cleanup in the finalizer to handle deletion of ApplicationDefinition resources
- Implemented garbage collection of all managed resources when an ApplicationDefinition has no components
- Added logic to remove component statuses that are no longer in the spec
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/controller/app/applicationdefinition_controller.go | Implements PVC cleanup in finalizer, adds resource garbage collection for empty apps, and removes stale component statuses |
| .gitignore | Adds .claude to ignored files for local development environment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clean up all PVCs associated with this appdef | ||
| logger.Info("Cleaning up all PVCs associated with the application") | ||
| pvcList := &corev1.PersistentVolumeClaimList{} | ||
| listOpts := []client.ListOption{ | ||
| client.InNamespace(appDef.Namespace), | ||
| client.MatchingLabels{ | ||
| appNameLabel: appDef.Name, // Match all PVCs labeled with this app name | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Client.List(ctx, pvcList, listOpts...); err != nil { | ||
| logger.Error(err, "Failed to list PVCs for cleanup") | ||
| return true, err | ||
| } | ||
|
|
||
| // Delete each PVC | ||
| for _, pvc := range pvcList.Items { | ||
| pvcCopy := pvc // Create a copy to avoid pointer issues | ||
| logger.Info("Deleting PVC", "name", pvcCopy.Name) | ||
| if err := r.Client.Delete(ctx, &pvcCopy); err != nil && !apierrors.IsNotFound(err) { | ||
| logger.Error(err, "Failed to delete PVC", "name", pvcCopy.Name) | ||
| return true, err | ||
| } | ||
| } |
There was a problem hiding this comment.
The new PVC cleanup logic in the finalizer lacks test coverage. The existing test file (applicationdefinition_controller_test.go) has tests for reconciliation and suspend/resume, but doesn't test the deletion flow with PVC cleanup.
Consider adding a test case that:
- Creates an ApplicationDefinition with components that create PVCs (StatefulSet with storage)
- Verifies PVCs are created
- Deletes the ApplicationDefinition
- Verifies that PVCs are properly cleaned up during finalizer execution
| // --- Garbage collect orphaned resources previously managed by this AppDef --- | ||
| // Delete all resources labeled with this app name | ||
| resourceTypes := []client.Object{ | ||
| &appsv1.Deployment{}, | ||
| &appsv1.StatefulSet{}, | ||
| &corev1.Service{}, | ||
| &corev1.PersistentVolumeClaim{}, | ||
| &corev1.ConfigMap{}, | ||
| &corev1.Secret{}, | ||
| &corev1.ServiceAccount{}, | ||
| } | ||
|
|
||
| for _, resourceType := range resourceTypes { | ||
| // Delete each resource type using DeleteAllOf | ||
| deleteOpts := []client.DeleteAllOfOption{ | ||
| client.InNamespace(state.appDef.Namespace), | ||
| client.MatchingLabels{ | ||
| appNameLabel: state.appDef.Name, | ||
| }, | ||
| } | ||
|
|
||
| // Delete all resources of this type with matching labels | ||
| if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil { | ||
| logger.Error(err, "Failed to garbage collect resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) | ||
| continue | ||
| } | ||
| logger.Info("Garbage collected resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) | ||
| } |
There was a problem hiding this comment.
The new garbage collection logic for orphaned resources in handleEmptyApp lacks test coverage. The existing test file has tests for reconciliation but doesn't test the scenario where an ApplicationDefinition transitions from having components to having no components.
Consider adding a test case that:
- Creates an ApplicationDefinition with components that create resources (Deployments, StatefulSets, Services, PVCs, etc.)
- Verifies resources are created
- Updates the ApplicationDefinition to have an empty components list
- Reconciles and verifies that all labeled resources are properly garbage collected
| if err := r.Client.List(ctx, pvcList, listOpts...); err != nil { | ||
| logger.Error(err, "Failed to list PVCs for cleanup") | ||
| return true, err | ||
| } | ||
|
|
||
| // Delete each PVC | ||
| for _, pvc := range pvcList.Items { | ||
| pvcCopy := pvc // Create a copy to avoid pointer issues | ||
| logger.Info("Deleting PVC", "name", pvcCopy.Name) | ||
| if err := r.Client.Delete(ctx, &pvcCopy); err != nil && !apierrors.IsNotFound(err) { | ||
| logger.Error(err, "Failed to delete PVC", "name", pvcCopy.Name) | ||
| return true, err | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider adding a log message indicating how many PVCs were found and cleaned up. This would provide better observability for operators debugging cleanup issues.
Example:
if err := r.Client.List(ctx, pvcList, listOpts...); err != nil {
logger.Error(err, "Failed to list PVCs for cleanup")
return true, err
}
logger.Info("Found PVCs to clean up", "count", len(pvcList.Items))
// Delete each PVC
for _, pvc := range pvcList.Items {
// ...
}
logger.Info("Successfully deleted all PVCs", "count", len(pvcList.Items))| if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil { | ||
| logger.Error(err, "Failed to garbage collect resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) | ||
| continue | ||
| } | ||
| logger.Info("Garbage collected resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) |
There was a problem hiding this comment.
The GetObjectKind().GroupVersionKind().Kind will return an empty string for typed objects when TypeMeta is not populated (which is common for Go typed objects). This will result in uninformative log messages showing empty resource types.
Consider using a type switch or reflection to get the actual type name:
for _, resourceType := range resourceTypes {
var typeName string
switch resourceType.(type) {
case *appsv1.Deployment:
typeName = "Deployment"
case *appsv1.StatefulSet:
typeName = "StatefulSet"
case *corev1.Service:
typeName = "Service"
case *corev1.PersistentVolumeClaim:
typeName = "PersistentVolumeClaim"
case *corev1.ConfigMap:
typeName = "ConfigMap"
case *corev1.Secret:
typeName = "Secret"
case *corev1.ServiceAccount:
typeName = "ServiceAccount"
default:
typeName = fmt.Sprintf("%T", resourceType)
}
deleteOpts := []client.DeleteAllOfOption{
client.InNamespace(state.appDef.Namespace),
client.MatchingLabels{
appNameLabel: state.appDef.Name,
},
}
if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil {
logger.Error(err, "Failed to garbage collect resources", "resourceType", typeName)
continue
}
logger.Info("Garbage collected resources", "resourceType", typeName)
}| if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil { | |
| logger.Error(err, "Failed to garbage collect resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) | |
| continue | |
| } | |
| logger.Info("Garbage collected resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) | |
| var typeName string | |
| switch resourceType.(type) { | |
| case *appsv1.Deployment: | |
| typeName = "Deployment" | |
| case *appsv1.StatefulSet: | |
| typeName = "StatefulSet" | |
| case *corev1.Service: | |
| typeName = "Service" | |
| case *corev1.PersistentVolumeClaim: | |
| typeName = "PersistentVolumeClaim" | |
| case *corev1.ConfigMap: | |
| typeName = "ConfigMap" | |
| case *corev1.Secret: | |
| typeName = "Secret" | |
| case *corev1.ServiceAccount: | |
| typeName = "ServiceAccount" | |
| default: | |
| typeName = fmt.Sprintf("%T", resourceType) | |
| } | |
| if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil { | |
| logger.Error(err, "Failed to garbage collect resources", "resourceType", typeName) | |
| continue | |
| } | |
| logger.Info("Garbage collected resources", "resourceType", typeName) |
| if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil { | ||
| logger.Error(err, "Failed to garbage collect resources", "resourceType", resourceType.GetObjectKind().GroupVersionKind().Kind) | ||
| continue |
There was a problem hiding this comment.
Using continue on deletion errors means that if one resource type fails to delete, the function will proceed to delete other resource types and then return success. This could leave orphaned resources and provide misleading feedback to the user.
Consider collecting errors and returning them, or at least tracking whether any deletions failed:
var deletionErrors []error
for _, resourceType := range resourceTypes {
// ... type name logic ...
if err := r.Client.DeleteAllOf(ctx, resourceType, deleteOpts...); err != nil {
logger.Error(err, "Failed to garbage collect resources", "resourceType", typeName)
deletionErrors = append(deletionErrors, fmt.Errorf("failed to delete %s: %w", typeName, err))
continue
}
logger.Info("Garbage collected resources", "resourceType", typeName)
}
// If there were any deletion errors, consider how to handle them
if len(deletionErrors) > 0 {
// Perhaps update condition with warning, or log aggregate error
logger.Info("Some resources failed to delete", "errorCount", len(deletionErrors))
}|
|
||
| // TODO: Implement logic to garbage collect orphaned resources previously managed by this AppDef? | ||
| // This is complex and relies on labels/ownerRefs. For now, assume GC handles it. | ||
| setCondition(state.appDef, metav1.Condition{Type: string(appv1.ConditionReady), Status: metav1.ConditionTrue, Reason: "NoComponentsDefined", Message: "Application has no components defined, orphaned resources cleaned up"}) |
There was a problem hiding this comment.
The condition message states "orphaned resources cleaned up" unconditionally, even if some or all deletion operations failed (due to the continue on line 340). This creates misleading status information.
The message should accurately reflect what happened. Consider updating it based on whether deletions were successful or track deletion failures to provide accurate status.
No description provided.