From 2fa52b99c85fd6596aeabc3e944ff2add1e8b2e9 Mon Sep 17 00:00:00 2001 From: Liqian Luo Date: Fri, 25 Jul 2025 23:20:13 +0000 Subject: [PATCH 1/2] feat: add source placement annotation to track workload source - Add setSourcePlacementAnnotation function to apply.go to annotate manifests with their source placement information - Update process.go to call setSourcePlacementAnnotation during manifest processing - Add comprehensive unit tests following established testing patterns - Update common.go with source placement annotation constant --- ...-07-25-0800-placement-source-annotation.md | 62 +++++++++ pkg/controllers/workapplier/apply.go | 41 ++++++ pkg/controllers/workapplier/apply_test.go | 124 ++++++++++++++++++ pkg/controllers/workapplier/process.go | 2 +- pkg/utils/common.go | 3 + 5 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 .github/.copilot/breadcrumbs/2025-07-25-0800-placement-source-annotation.md diff --git a/.github/.copilot/breadcrumbs/2025-07-25-0800-placement-source-annotation.md b/.github/.copilot/breadcrumbs/2025-07-25-0800-placement-source-annotation.md new file mode 100644 index 000000000..e3e0ba934 --- /dev/null +++ b/.github/.copilot/breadcrumbs/2025-07-25-0800-placement-source-annotation.md @@ -0,0 +1,62 @@ +# Placement Source Annotation Implementation + +## Context +Add an annotation to all applied resources in the Work controller to track what source placement resource created them. This will help with debugging, resource tracking, and understanding the origin of deployed resources. + +## Analysis Phase + +### Current Understanding +- Work controller applies manifests to member clusters through `ApplyWorkReconciler` +- Resources are applied via two strategies: client-side and server-side apply +- Work objects contain a label `fleetv1beta1.CRPTrackingLabel` that tracks the placement +- Applied resources currently get owner references but no source placement annotation + +### Implementation Options + +#### Option 1: Add annotation during manifest preparation in apply_controller.go +- **Pros**: Centralized location, affects all appliers +- **Cons**: Requires passing placement info through the call chain + +#### Option 2: Add annotation in each applier implementation +- **Pros**: More granular control, easier to implement +- **Cons**: Need to modify both client-side and server-side appliers + +#### Option 3: Add annotation in the applyManifests function +- **Pros**: Single point of change, before any applier is called +- **Cons**: Need to extract placement info from Work object + +### Questions and Decisions +1. **Annotation key**: Use `fleet.azure.com/source-placement` to follow existing pattern in `utils/common.go` +2. **Value format**: Need both the name of the placement and its namespace as we want to support both ResourcePlacement and ClusterResourcePlacement. +3. **Missing label handling**: Log warning and skip annotation if placement label is missing +4. **Placement info location**: Available on Work objects via `fleetv1beta1.PlacementTrackingLabel` label + +## Implementation Plan + +### Phase 1: Define Annotation Constant + +- [x] Add annotation key constant to APIs +- [ ] Decide on annotation format and value + +### Phase 2: Extract Placement Information +- [x] Modify applyManifests to extract placement from Work labels +- [x] Handle cases where placement label might be missing + +### Phase 3: Add Annotation to Manifests +- [x] Modify manifest objects before applying them +- [x] Ensure annotation is added consistently across both applier types + +### Phase 4: Testing +- [x] Write unit tests for annotation addition +- [ ] Test with both client-side and server-side apply strategies +- [ ] Verify annotation appears on deployed resources + +### Phase 5: Integration Testing +- [ ] Test end-to-end with actual placements +- [ ] Verify backwards compatibility + +## Success Criteria +- All applied resources have the source placement annotation +- Annotation is correctly set for both apply strategies +- No breaking changes to existing functionality +- Tests pass and cover the new functionality diff --git a/pkg/controllers/workapplier/apply.go b/pkg/controllers/workapplier/apply.go index 4e447e4bf..8169de3c2 100644 --- a/pkg/controllers/workapplier/apply.go +++ b/pkg/controllers/workapplier/apply.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" + "github.com/kubefleet-dev/kubefleet/pkg/utils" "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" "github.com/kubefleet-dev/kubefleet/pkg/utils/resource" ) @@ -69,6 +70,7 @@ func (r *Reconciler) apply( manifestObj, inMemberClusterObj *unstructured.Unstructured, applyStrategy *fleetv1beta1.ApplyStrategy, expectedAppliedWorkOwnerRef *metav1.OwnerReference, + work *fleetv1beta1.Work, ) (*unstructured.Unstructured, error) { // Create a sanitized copy of the manifest object. // @@ -96,6 +98,11 @@ func (r *Reconciler) apply( return nil, fmt.Errorf("failed to set manifest hash annotation: %w", err) } + // Add the source placement annotation to track the placement that created this resource. + if err := setSourcePlacementAnnotation(manifestObjCopy, work); err != nil { + return nil, fmt.Errorf("failed to set source placement annotation: %w", err) + } + // Validate owner references. // // As previously mentioned, with the new capabilities, at this point of the workflow, @@ -457,6 +464,40 @@ func setManifestHashAnnotation(manifestObj *unstructured.Unstructured) error { return nil } +// setSourcePlacementAnnotation sets the source placement annotation on the provided unstructured object. +func setSourcePlacementAnnotation(manifestObj *unstructured.Unstructured, work *fleetv1beta1.Work) error { + // Extract the placement name from the Work object's placement tracking label. + workLabels := work.GetLabels() + if workLabels == nil { + // If there are no labels, we cannot determine the source placement. + // This is not an error case as some Work objects might not have placement tracking. + klog.V(2).InfoS("Work object has no labels, skipping source placement annotation", + "work", klog.KObj(work)) + return nil + } + + placementName, exists := workLabels[fleetv1beta1.PlacementTrackingLabel] + if !exists || placementName == "" { + // If the placement tracking label is missing or empty, we cannot determine the source placement. + // This is not an error case as some Work objects might not be created by a placement. + klog.V(2).InfoS("Work object has no placement tracking label, skipping source placement annotation", + "work", klog.KObj(work)) + return nil + } + + // Set the source placement annotation on the manifest object. + annotations := manifestObj.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[utils.SourcePlacementAnnotation] = placementName + manifestObj.SetAnnotations(annotations) + + klog.V(2).InfoS("Set source placement annotation on manifest", + "manifest", klog.KObj(manifestObj), "placement", placementName) + return nil +} + // setOwnerRef sets the expected owner reference (reference to an AppliedWork object) // on a manifest to be applied. func setOwnerRef(obj *unstructured.Unstructured, expectedAppliedWorkOwnerRef *metav1.OwnerReference) { diff --git a/pkg/controllers/workapplier/apply_test.go b/pkg/controllers/workapplier/apply_test.go index f73bac0bf..355d84ecf 100644 --- a/pkg/controllers/workapplier/apply_test.go +++ b/pkg/controllers/workapplier/apply_test.go @@ -31,6 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" + "github.com/kubefleet-dev/kubefleet/pkg/utils" ) // Note (chenyu1): The fake client Fleet uses for unit tests has trouble processing certain requests @@ -550,3 +551,126 @@ func TestShouldUseForcedServerSideApply(t *testing.T) { }) } } + +// TestSetSourcePlacementAnnotation tests the setSourcePlacementAnnotation function. +func TestSetSourcePlacementAnnotation(t *testing.T) { + // Base namespace object to use for testing + nsName := "ns-1" + ns := &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Namespace", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: nsName, + }, + } + + // Test case 1: Work with placement tracking label + workWithPlacement := &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-work", + Namespace: "test-namespace", + Labels: map[string]string{ + fleetv1beta1.PlacementTrackingLabel: "test-placement", + }, + }, + } + manifestObj1 := toUnstructured(t, ns.DeepCopy()) + wantManifestObj1 := toUnstructured(t, ns.DeepCopy()) + wantManifestObj1.SetAnnotations(map[string]string{ + utils.SourcePlacementAnnotation: "test-placement", + }) + + // Test case 2: Work without placement tracking label (no labels at all) + workWithoutLabels := &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-work-no-labels", + Namespace: "test-namespace", + }, + } + manifestObj2 := toUnstructured(t, ns.DeepCopy()) + wantManifestObj2 := toUnstructured(t, ns.DeepCopy()) + // No annotations should be added + + // Test case 3: Work with empty placement tracking label + workWithEmptyPlacement := &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-work-empty-placement", + Namespace: "test-namespace", + Labels: map[string]string{ + fleetv1beta1.PlacementTrackingLabel: "", + }, + }, + } + manifestObj3 := toUnstructured(t, ns.DeepCopy()) + wantManifestObj3 := toUnstructured(t, ns.DeepCopy()) + // No annotations should be added + + // Test case 4: Manifest with existing annotations + workWithPlacement2 := &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-work-2", + Namespace: "test-namespace", + Labels: map[string]string{ + fleetv1beta1.PlacementTrackingLabel: "another-placement", + }, + }, + } + manifestObj4 := toUnstructured(t, ns.DeepCopy()) + manifestObj4.SetAnnotations(map[string]string{ + "existing.annotation": "existing-value", + "another.annotation": "another-value", + }) + wantManifestObj4 := toUnstructured(t, ns.DeepCopy()) + wantManifestObj4.SetAnnotations(map[string]string{ + "existing.annotation": "existing-value", + "another.annotation": "another-value", + utils.SourcePlacementAnnotation: "another-placement", + }) + + testCases := []struct { + name string + work *fleetv1beta1.Work + manifestObj *unstructured.Unstructured + wantManifestObj *unstructured.Unstructured + }{ + { + name: "work with placement tracking label", + work: workWithPlacement, + manifestObj: manifestObj1, + wantManifestObj: wantManifestObj1, + }, + { + name: "work without labels", + work: workWithoutLabels, + manifestObj: manifestObj2, + wantManifestObj: wantManifestObj2, + }, + { + name: "work with empty placement tracking label", + work: workWithEmptyPlacement, + manifestObj: manifestObj3, + wantManifestObj: wantManifestObj3, + }, + { + name: "manifest with existing annotations", + work: workWithPlacement2, + manifestObj: manifestObj4, + wantManifestObj: wantManifestObj4, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := setSourcePlacementAnnotation(tc.manifestObj, tc.work) + if err != nil { + t.Fatalf("setSourcePlacementAnnotation() = %v, want no error", err) + } + + if diff := cmp.Diff(tc.manifestObj, tc.wantManifestObj); diff != "" { + t.Errorf("manifest obj mismatches (-got +want):\n%s", diff) + } + }) + } +} diff --git a/pkg/controllers/workapplier/process.go b/pkg/controllers/workapplier/process.go index 9b660e528..56c21633f 100644 --- a/pkg/controllers/workapplier/process.go +++ b/pkg/controllers/workapplier/process.go @@ -107,7 +107,7 @@ func (r *Reconciler) processOneManifest( } // Perform the apply op. - appliedObj, err := r.apply(ctx, bundle.gvr, bundle.manifestObj, bundle.inMemberClusterObj, work.Spec.ApplyStrategy, expectedAppliedWorkOwnerRef) + appliedObj, err := r.apply(ctx, bundle.gvr, bundle.manifestObj, bundle.inMemberClusterObj, work.Spec.ApplyStrategy, expectedAppliedWorkOwnerRef, work) if err != nil { bundle.applyErr = fmt.Errorf("failed to apply the manifest: %w", err) bundle.applyResTyp = ManifestProcessingApplyResultTypeFailedToApply diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 507b3260d..e3de49d70 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -109,6 +109,9 @@ const ( // FleetAnnotationPrefix is the prefix used to annotate fleet member cluster resources. FleetAnnotationPrefix = "fleet.azure.com" + + // SourcePlacementAnnotation is the annotation key used to track the source placement of applied resources. + SourcePlacementAnnotation = FleetAnnotationPrefix + "/source-placement" ) var ( From e522379887d03f9c96429f7c2fa0e06de9ed9023 Mon Sep 17 00:00:00 2001 From: Liqian Luo Date: Wed, 30 Jul 2025 18:03:24 +0000 Subject: [PATCH 2/2] Use the new prefix instead of fleet.azure.com Signed-off-by: Liqian Luo --- apis/placement/v1beta1/work_types.go | 3 +++ pkg/controllers/workapplier/apply.go | 3 +-- pkg/controllers/workapplier/apply_test.go | 9 ++++----- pkg/utils/common.go | 3 --- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/apis/placement/v1beta1/work_types.go b/apis/placement/v1beta1/work_types.go index d1339ac7b..83e6c854d 100644 --- a/apis/placement/v1beta1/work_types.go +++ b/apis/placement/v1beta1/work_types.go @@ -45,6 +45,9 @@ const ( // LastAppliedConfigAnnotation is to record the last applied configuration on the object. LastAppliedConfigAnnotation = fleetPrefix + "last-applied-configuration" + // SourcePlacementAnnotation is the annotation key used to track the source placement of applied resources. + SourcePlacementAnnotation = fleetPrefix + "source-placement" + // WorkConditionTypeApplied represents workload in Work is applied successfully on the spoke cluster. WorkConditionTypeApplied = "Applied" diff --git a/pkg/controllers/workapplier/apply.go b/pkg/controllers/workapplier/apply.go index 8169de3c2..2d8a2b122 100644 --- a/pkg/controllers/workapplier/apply.go +++ b/pkg/controllers/workapplier/apply.go @@ -35,7 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" - "github.com/kubefleet-dev/kubefleet/pkg/utils" "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" "github.com/kubefleet-dev/kubefleet/pkg/utils/resource" ) @@ -490,7 +489,7 @@ func setSourcePlacementAnnotation(manifestObj *unstructured.Unstructured, work * if annotations == nil { annotations = map[string]string{} } - annotations[utils.SourcePlacementAnnotation] = placementName + annotations[fleetv1beta1.SourcePlacementAnnotation] = placementName manifestObj.SetAnnotations(annotations) klog.V(2).InfoS("Set source placement annotation on manifest", diff --git a/pkg/controllers/workapplier/apply_test.go b/pkg/controllers/workapplier/apply_test.go index 355d84ecf..0abcab488 100644 --- a/pkg/controllers/workapplier/apply_test.go +++ b/pkg/controllers/workapplier/apply_test.go @@ -31,7 +31,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" - "github.com/kubefleet-dev/kubefleet/pkg/utils" ) // Note (chenyu1): The fake client Fleet uses for unit tests has trouble processing certain requests @@ -579,7 +578,7 @@ func TestSetSourcePlacementAnnotation(t *testing.T) { manifestObj1 := toUnstructured(t, ns.DeepCopy()) wantManifestObj1 := toUnstructured(t, ns.DeepCopy()) wantManifestObj1.SetAnnotations(map[string]string{ - utils.SourcePlacementAnnotation: "test-placement", + fleetv1beta1.SourcePlacementAnnotation: "test-placement", }) // Test case 2: Work without placement tracking label (no labels at all) @@ -624,9 +623,9 @@ func TestSetSourcePlacementAnnotation(t *testing.T) { }) wantManifestObj4 := toUnstructured(t, ns.DeepCopy()) wantManifestObj4.SetAnnotations(map[string]string{ - "existing.annotation": "existing-value", - "another.annotation": "another-value", - utils.SourcePlacementAnnotation: "another-placement", + "existing.annotation": "existing-value", + "another.annotation": "another-value", + fleetv1beta1.SourcePlacementAnnotation: "another-placement", }) testCases := []struct { diff --git a/pkg/utils/common.go b/pkg/utils/common.go index e3de49d70..507b3260d 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -109,9 +109,6 @@ const ( // FleetAnnotationPrefix is the prefix used to annotate fleet member cluster resources. FleetAnnotationPrefix = "fleet.azure.com" - - // SourcePlacementAnnotation is the annotation key used to track the source placement of applied resources. - SourcePlacementAnnotation = FleetAnnotationPrefix + "/source-placement" ) var (