interface: Add deletion behavior to ClusterResourcePlacement API (#140)

Copilot · web-flow · commit e7d7077b3de0 · 2025-07-18T13:00:07.000-07:00
diff --git a/.github/.copilot/breadcrumbs/2025-01-15-1400-crp-delete-policy.md b/.github/.copilot/breadcrumbs/2025-01-15-1400-crp-delete-policy.md
@@ -0,0 +1,77 @@
+# CRP Delete Policy Implementation
+
+## Problem Analysis
+
+The issue requests adding API options to allow customers to choose whether to delete placed resources when a CRP (ClusterResourcePlacement) is deleted.
+
+### Current Deletion Behavior
+1. When a CRP is deleted, it has two finalizers:
+   - `ClusterResourcePlacementCleanupFinalizer` - handled by CRP controller to delete snapshots
+   - `SchedulerCleanupFinalizer` - handled by scheduler to delete bindings
+   
+2. The deletion flow:
+   - CRP controller removes snapshots (ClusterSchedulingPolicySnapshot, ClusterResourceSnapshot)
+   - Scheduler removes bindings (ClusterResourceBinding) which triggers cleanup of placed resources
+   - Currently there's no option for users to control whether placed resources are deleted
+
+### References
+- Kubernetes DeleteOptions has `PropagationPolicy` with values: `Orphan`, `Background`, `Foreground`
+- AKS API has `DeletePolicy` pattern (couldn't fetch exact details but following similar pattern)
+
+## Implementation Plan
+
+### Phase 1: API Design
+- [x] Add `DeleteStrategy` struct to `RolloutStrategy` in beta API
+- [x] Define `PropagationPolicy` field with enum values: `Delete` (default), `Abandon`
+- [x] Update API documentation and validation
+- [x] Move DeleteStrategy inside RolloutStrategy after ApplyStrategy for consistency
+
+### Phase 2: Implementation Details
+TODO: @Arvindthiru to fill out the details for controller logic implementation.
+
+### Phase 3: Testing
+- [ ] Add unit tests for new deletion policy options
+- [ ] Add integration tests to verify behavior  
+- [ ] Test both `Delete` and `Abandon` scenarios
+
+### Phase 4: Documentation & Examples
+- [ ] Update CRD documentation
+- [ ] Add example configurations
+- [ ] Update any user-facing documentation
+
+## Success Criteria
+- [x] CRP API has `deleteStrategy` field with `Delete`/`Abandon` options inside RolloutStrategy
+- [x] Default behavior (`Delete`) preserves current functionality
+- [ ] `Abandon` policy leaves placed resources intact when CRP is deleted
+- [ ] All tests pass including new deletion policy tests
+- [x] Changes are minimal and backwards compatible
+
+## Current API Structure
+
+The DeleteStrategy is now part of RolloutStrategy:
+
+```go
+type RolloutStrategy struct {
+    // ... other fields ...
+    ApplyStrategy *ApplyStrategy `json:"applyStrategy,omitempty"`
+    DeleteStrategy *DeleteStrategy `json:"deleteStrategy,omitempty"`
+}
+
+type DeleteStrategy struct {
+    PropagationPolicy DeletePropagationPolicy `json:"propagationPolicy,omitempty"`
+}
+
+type DeletePropagationPolicy string
+
+const (
+    DeletePropagationPolicyDelete DeletePropagationPolicy = "Delete"    // default
+    DeletePropagationPolicyAbandon DeletePropagationPolicy = "Abandon"
+)
+```
+
+## Behavior Summary
+
+- **Default (`Delete`)**: When CRP is deleted, all placed resources are removed from member clusters (current behavior)
+- **Abandon**: When CRP is deleted, placed resources remain on member clusters but are no longer managed by Fleet
+
+This provides customers with the flexibility to choose between complete cleanup or leaving resources in place when deleting a CRP.
diff --git a/apis/placement/v1beta1/clusterresourceplacement_types.go b/apis/placement/v1beta1/clusterresourceplacement_types.go
@@ -488,6 +488,10 @@ type RolloutStrategy struct {
 	// ApplyStrategy describes when and how to apply the selected resources to the target cluster.
 	// +kubebuilder:validation:Optional
 	ApplyStrategy *ApplyStrategy `json:"applyStrategy,omitempty"`
+
+	// DeleteStrategy configures the deletion behavior when the ClusterResourcePlacement is deleted.
+	// +kubebuilder:validation:Optional
+	DeleteStrategy *DeleteStrategy `json:"deleteStrategy,omitempty"`
 }
 
 // ApplyStrategy describes when and how to apply the selected resource to the target cluster.
@@ -1319,6 +1323,38 @@ const (
 	PickFixedPlacementType PlacementType = "PickFixed"
 )
 
+// DeleteStrategy configures the deletion behavior when a placement is deleted.
+type DeleteStrategy struct {
+	// PropagationPolicy controls whether to delete placed resources when placement is deleted.
+	//
+	// Available options:
+	//
+	// * Delete: all placed resources on member clusters will be deleted when
+	//   the placement is deleted. This is the default behavior.
+	//
+	// * Abandon: all placed resources on member clusters will be left intact (abandoned)
+	//   when the placement is deleted.
+	//
+	// +kubebuilder:validation:Enum=Abandon;Delete
+	// +kubebuilder:default=Delete
+	// +kubebuilder:validation:Optional
+	PropagationPolicy DeletePropagationPolicy `json:"propagationPolicy,omitempty"`
+}
+
+// DeletePropagationPolicy identifies the propagation policy when a placement is deleted.
+// +enum
+type DeletePropagationPolicy string
+
+const (
+	// DeletePropagationPolicyAbandon instructs Fleet to leave (abandon) all placed resources on member
+	// clusters when the placement is deleted.
+	DeletePropagationPolicyAbandon DeletePropagationPolicy = "Abandon"
+
+	// DeletePropagationPolicyDelete instructs Fleet to delete all placed resources on member clusters
+	// when the placement is deleted. This is the default behavior.
+	DeletePropagationPolicyDelete DeletePropagationPolicy = "Delete"
+)
+
 // ClusterResourcePlacementList contains a list of ClusterResourcePlacement.
 // +kubebuilder:resource:scope="Cluster"
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
diff --git a/apis/placement/v1beta1/zz_generated.deepcopy.go b/apis/placement/v1beta1/zz_generated.deepcopy.go
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterresourcebindings.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterresourcebindings.yaml
@@ -542,8 +542,8 @@ spec:
                         managed by Fleet (i.e., specified in the hub cluster manifest). This is the default
                         option.
 
-                        Note that this option would revert any ad-hoc changes made on the member cluster side in
-                        the managed fields; if you would like to make temporary edits on the member cluster side
+                        Note that this option would revert any ad-hoc changes made on the member cluster side in the
+                        managed fields; if you would like to make temporary edits on the member cluster side
                         in the managed fields, switch to IfNotDrifted option. Note that changes in unmanaged
                         fields will be left alone; if you use the FullDiff compare option, such changes will
                         be reported as drifts.
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml
@@ -1905,6 +1905,28 @@ spec:
                         - Never
                         type: string
                     type: object
+                  deleteStrategy:
+                    description: DeleteStrategy configures the deletion behavior when
+                      the ClusterResourcePlacement is deleted.
+                    properties:
+                      propagationPolicy:
+                        default: Delete
+                        description: |-
+                          PropagationPolicy controls how the deletion is propagated to placed resources.
+                          Defaults to "Delete".
+
+                          Available options:
+
+                          * Delete: all placed resources on member clusters will be deleted when the
+                            ClusterResourcePlacement is deleted. This is the default behavior.
+
+                          * Abandon: all placed resources on member clusters will be left intact (abandoned)
+                            when the ClusterResourcePlacement is deleted.
+                        enum:
+                        - Abandon
+                        - Delete
+                        type: string
+                    type: object
                   rollingUpdate:
                     description: Rolling update config params. Present only if RolloutStrategyType
                       = RollingUpdate.
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterresourcesnapshots.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterresourcesnapshots.yaml
@@ -40,6 +40,8 @@ spec:
           Each snapshot MUST have the following labels:
             - `CRPTrackingLabel` which points to its owner CRP.
             - `ResourceIndexLabel` which is the index  of the snapshot group.
+
+          The first snapshot of the index group MAY have the following labels:
             - `IsLatestSnapshotLabel` which indicates whether the snapshot is the latest one.
 
           All the snapshots within the same index group must have the same ResourceIndexLabel.
@@ -50,6 +52,9 @@ spec:
 
           Each snapshot (excluding the first snapshot) MUST have the following annotations:
             - `SubindexOfResourceSnapshotAnnotation` to store the subindex of resource snapshot in the group.
+
+          Snapshot may have the following annotations to indicate the time of next resourceSnapshot candidate detected by the controller:
+            - `NextResourceSnapshotCandidateDetectionTimeAnnotation` to store the time of next resourceSnapshot candidate detected by the controller.
         properties:
           apiVersion:
             description: |-
@@ -186,6 +191,9 @@ spec:
 
           Each snapshot (excluding the first snapshot) MUST have the following annotations:
             - `SubindexOfResourceSnapshotAnnotation` to store the subindex of resource snapshot in the group.
+
+          Snapshot may have the following annotations to indicate the time of next resourceSnapshot candidate detected by the controller:
+            - `NextResourceSnapshotCandidateDetectionTimeAnnotation` to store the time of next resourceSnapshot candidate detected by the controller.
         properties:
           apiVersion:
             description: |-
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml
@@ -235,8 +235,8 @@ spec:
                         managed by Fleet (i.e., specified in the hub cluster manifest). This is the default
                         option.
 
-                        Note that this option would revert any ad-hoc changes made on the member cluster side in
-                        the managed fields; if you would like to make temporary edits on the member cluster side
+                        Note that this option would revert any ad-hoc changes made on the member cluster side in the
+                        managed fields; if you would like to make temporary edits on the member cluster side
                         in the managed fields, switch to IfNotDrifted option. Note that changes in unmanaged
                         fields will be left alone; if you use the FullDiff compare option, such changes will
                         be reported as drifts.
@@ -1315,8 +1315,8 @@ spec:
                         managed by Fleet (i.e., specified in the hub cluster manifest). This is the default
                         option.
 
-                        Note that this option would revert any ad-hoc changes made on the member cluster side in
-                        the managed fields; if you would like to make temporary edits on the member cluster side
+                        Note that this option would revert any ad-hoc changes made on the member cluster side in the
+                        managed fields; if you would like to make temporary edits on the member cluster side
                         in the managed fields, switch to IfNotDrifted option. Note that changes in unmanaged
                         fields will be left alone; if you use the FullDiff compare option, such changes will
                         be reported as drifts.
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_resourcebindings.yaml b/config/crd/bases/placement.kubernetes-fleet.io_resourcebindings.yaml
@@ -190,8 +190,8 @@ spec:
                         managed by Fleet (i.e., specified in the hub cluster manifest). This is the default
                         option.
 
-                        Note that this option would revert any ad-hoc changes made on the member cluster side in
-                        the managed fields; if you would like to make temporary edits on the member cluster side
+                        Note that this option would revert any ad-hoc changes made on the member cluster side in the
+                        managed fields; if you would like to make temporary edits on the member cluster side
                         in the managed fields, switch to IfNotDrifted option. Note that changes in unmanaged
                         fields will be left alone; if you use the FullDiff compare option, such changes will
                         be reported as drifts.
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml b/config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml
@@ -742,8 +742,8 @@ spec:
                             managed by Fleet (i.e., specified in the hub cluster manifest). This is the default
                             option.
 
-                            Note that this option would revert any ad-hoc changes made on the member cluster side in
-                            the managed fields; if you would like to make temporary edits on the member cluster side
+                            Note that this option would revert any ad-hoc changes made on the member cluster side in the
+                            managed fields; if you would like to make temporary edits on the member cluster side
                             in the managed fields, switch to IfNotDrifted option. Note that changes in unmanaged
                             fields will be left alone; if you use the FullDiff compare option, such changes will
                             be reported as drifts.
@@ -840,6 +840,28 @@ spec:
                         - Never
                         type: string
                     type: object
+                  deleteStrategy:
+                    description: DeleteStrategy configures the deletion behavior when
+                      the ClusterResourcePlacement is deleted.
+                    properties:
+                      propagationPolicy:
+                        default: Delete
+                        description: |-
+                          PropagationPolicy controls how the deletion is propagated to placed resources.
+                          Defaults to "Delete".
+
+                          Available options:
+
+                          * Delete: all placed resources on member clusters will be deleted when the
+                            ClusterResourcePlacement is deleted. This is the default behavior.
+
+                          * Abandon: all placed resources on member clusters will be left intact (abandoned)
+                            when the ClusterResourcePlacement is deleted.
+                        enum:
+                        - Abandon
+                        - Delete
+                        type: string
+                    type: object
                   rollingUpdate:
                     description: Rolling update config params. Present only if RolloutStrategyType
                       = RollingUpdate.
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_resourcesnapshots.yaml b/config/crd/bases/placement.kubernetes-fleet.io_resourcesnapshots.yaml
@@ -40,6 +40,8 @@ spec:
           Each snapshot MUST have the following labels:
             - `CRPTrackingLabel` which points to its owner resource placement.
             - `ResourceIndexLabel` which is the index  of the snapshot group.
+
+          The first snapshot of the index group MAY have the following labels:
             - `IsLatestSnapshotLabel` which indicates whether the snapshot is the latest one.
 
           All the snapshots within the same index group must have the same ResourceIndexLabel.
diff --git a/config/crd/bases/placement.kubernetes-fleet.io_works.yaml b/config/crd/bases/placement.kubernetes-fleet.io_works.yaml
@@ -433,8 +433,8 @@ spec:
                         managed by Fleet (i.e., specified in the hub cluster manifest). This is the default
                         option.
 
-                        Note that this option would revert any ad-hoc changes made on the member cluster side in
-                        the managed fields; if you would like to make temporary edits on the member cluster side
+                        Note that this option would revert any ad-hoc changes made on the member cluster side in the
+                        managed fields; if you would like to make temporary edits on the member cluster side
                         in the managed fields, switch to IfNotDrifted option. Note that changes in unmanaged
                         fields will be left alone; if you use the FullDiff compare option, such changes will
                         be reported as drifts.
