Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions .github/.copilot/breadcrumbs/2025-01-15-1400-crp-delete-policy.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 2 additions & 1 deletion .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ The main idea is that we are creating a multi-cluster application management sol
- Follow the [Uber Go Style Guide](https://github.com/uber-go/guide/blob/master/style.md) if possible.
- Favor using the standard library over third-party libraries.
- Run "make reviewable" before submitting a pull request to ensure the code is formatted correctly and all dependencies are up to date.
- The title of a PR must use one of the following prefixes: "[WIP] ", "feat: ", "test: ", "fix: ", "docs: ", "style: ", "interface: ", "util: ", "chore: ", "ci: ", "perf: ", "refactor: ", "revert: ". Please pick one that matches the PR content the most.

## Terminology
- **Fleet**: A conceptual term referring to a collection of clusters.
Expand Down Expand Up @@ -216,4 +217,4 @@ This practice creates a trail of decision points that document our thought proce

**User**: This looks good.

**Agent**: I've updated the breadcrumb with the latest understanding.
**Agent**: I've updated the breadcrumb with the latest understanding.
10 changes: 8 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,16 @@ jobs:
strategy:
fail-fast: false
matrix:
customized-settings: [default, custom]
customized-settings: [default, joinleave, custom]
include:
- customized-settings: default
# to shorten the test duration, set the resource snapshot creation interval to 0
resource-snapshot-creation-minimum-interval: 0m
resource-changes-collection-duration: 0m
- customized-settings: joinleave
# to shorten the test duration, set the resource snapshot creation interval to 0
resource-snapshot-creation-minimum-interval: 0m
resource-changes-collection-duration: 0m
- customized-settings: custom
resource-snapshot-creation-minimum-interval: 30s
resource-changes-collection-duration: 15s
Expand Down Expand Up @@ -132,7 +136,9 @@ jobs:
- name: Run e2e tests
run: |
if [ "${{ matrix.customized-settings }}" = "default" ]; then
make e2e-tests
make e2e-tests LABEL_FILTER="!custom && !joinleave"
elif [ "${{ matrix.customized-settings }}" = "joinleave" ]; then
make e2e-tests LABEL_FILTER="!custom && joinleave"
else
make e2e-tests-custom
fi
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/codespell.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Harden Runner
uses: step-security/harden-runner@6c439dc8bdf85cadbbce9ed30d1c7b959517bc49 # v2.12.2
uses: step-security/harden-runner@ec9f2d5744a09debf3a187a3f4f675c53b671911 # v2.13.0
with:
egress-policy: audit

Expand Down
2 changes: 2 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ make setup-clusters

# Run E2E tests with custom cluster count
make setup-clusters MEMBER_CLUSTER_COUNT=5

# Run parallel E2E tests (default - excludes custom tests)
make e2e-tests

# Clean up test clusters
Expand Down
5 changes: 4 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,12 @@ install-helm: load-hub-docker-image load-member-docker-image install-member-agen
.PHONY: e2e-tests-v1alpha1
e2e-tests-v1alpha1: create-kind-cluster run-e2e-v1alpha1

# E2E test label filter (can be overridden)
LABEL_FILTER ?= !custom

.PHONY: e2e-tests
e2e-tests: setup-clusters
cd ./test/e2e && ginkgo --timeout=70m --label-filter="!custom" -v -p .
cd ./test/e2e && ginkgo --timeout=70m --label-filter="$(LABEL_FILTER)" -v -p .

e2e-tests-custom: setup-clusters
cd ./test/e2e && ginkgo --label-filter="custom" -v -p .
Expand Down
29 changes: 0 additions & 29 deletions apis/placement/v1alpha1/common.go

This file was deleted.

16 changes: 0 additions & 16 deletions apis/placement/v1alpha1/overridesnapshot_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,6 @@ package v1alpha1

import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

const (

// OverrideIndexLabel is the label that indicate the policy snapshot index of a cluster policy.
OverrideIndexLabel = fleetPrefix + "override-index"

// OverrideSnapshotNameFmt is clusterResourceOverrideSnapshot name format: {CROName}-{OverrideSnapshotIndex}.
OverrideSnapshotNameFmt = "%s-%d"

// OverrideTrackingLabel is the label that points to the cluster resource override that creates a resource snapshot.
OverrideTrackingLabel = fleetPrefix + "parent-resource-override"

// OverrideFinalizer is a finalizer added by the override controllers to all override, to make sure
// that the override controller can react to override deletions if necessary.
OverrideFinalizer = fleetPrefix + "override-cleanup"
)

// +genclient
// +genclient:nonNamespaced
// +kubebuilder:object:root=true
Expand Down
36 changes: 36 additions & 0 deletions apis/placement/v1beta1/clusterresourceplacement_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
26 changes: 26 additions & 0 deletions apis/placement/v1beta1/commons.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,32 @@ const (
ApprovalTaskNameFmt = "%s-%s"
)

var (
// ClusterResourceOverrideKind is the kind of the ClusterResourceOverride.
ClusterResourceOverrideKind = "ClusterResourceOverride"

// ClusterResourceOverrideSnapshotKind is the kind of the ClusterResourceOverrideSnapshot.
ClusterResourceOverrideSnapshotKind = "ClusterResourceOverrideSnapshot"

// ResourceOverrideKind is the kind of the ResourceOverride.
ResourceOverrideKind = "ResourceOverride"

// ResourceOverrideSnapshotKind is the kind of the ResourceOverrideSnapshot.
ResourceOverrideSnapshotKind = "ResourceOverrideSnapshot"

// OverrideClusterNameVariable is the reserved variable in the override value that will be replaced by the actual cluster name.
OverrideClusterNameVariable = "${MEMBER-CLUSTER-NAME}"

// OverrideClusterLabelKeyVariablePrefix is a reserved variable in the override expression.
// We use this variable to find the associated the key following the prefix.
// The key name ends with a "}" character (but not include it).
// The key name must be a valid Kubernetes label name and case-sensitive.
// The content of the string containing this variable will be replaced by the actual label value on the member cluster.
// For example, if the string is "${MEMBER-CLUSTER-LABEL-KEY-kube-fleet.io/region}" then the key name is "kube-fleet.io/region".
// If there is a label "kube-fleet.io/region": "us-west-1" on the member cluster, this string will be replaced by "us-west-1".
OverrideClusterLabelKeyVariablePrefix = "${MEMBER-CLUSTER-LABEL-KEY-"
)

// NamespacedName comprises a resource name, with a mandatory namespace.
type NamespacedName struct {
// Name is the name of the namespaced scope resource.
Expand Down
Loading
Loading