track work deletion in binding as applied failed

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

Author: britaniar

pkg/controllers/workgenerator/controller.go

@@ -155,11 +155,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques
 
 	workUpdated := false
 	overrideSucceeded := false
+	workDeleted := false
 	// list all the corresponding works
 	works, syncErr := r.listAllWorksAssociated(ctx, &resourceBinding)
 	if syncErr == nil {
 		// generate and apply the workUpdated works if we have all the works
-		overrideSucceeded, workUpdated, syncErr = r.syncAllWork(ctx, &resourceBinding, works, &cluster)
+		overrideSucceeded, workUpdated, workDeleted, syncErr = r.syncAllWork(ctx, &resourceBinding, works, &cluster)
 	}
 	// Reset the conditions and failed/drifted/diffed placements.
 	for i := condition.OverriddenCondition; i < condition.TotalCondition; i++ {
@@ -220,10 +221,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques
 			Message:            "All of the works are synchronized to the latest",
 		})
 		switch {
+		case workDeleted:
+			// Some Work object(s) are being deleted; set a False Applied condition which signals
+			// that resources are in the process of being applied.
+			klog.V(2).InfoS("Some work being deleted", "resourceBinding", bindingRef)
+			setBindingStatus(workDeleted, works, &resourceBinding)
+			syncErr = controller.NewUserError(fmt.Errorf("some work objects are being deleted"))
 		case !workUpdated:
 			// The Work object itself is unchanged; refresh the cluster resource binding status
 			// based on the status information reported on the Work object(s).
-			setBindingStatus(works, &resourceBinding)
+			setBindingStatus(workDeleted, works, &resourceBinding)
 		case resourceBinding.Spec.ApplyStrategy == nil || resourceBinding.Spec.ApplyStrategy.Type != fleetv1beta1.ApplyStrategyTypeReportDiff:
 			// The Work object itself has changed; set a False Applied condition which signals
 			// that resources are in the process of being applied.
@@ -409,8 +416,9 @@ func (r *Reconciler) listAllWorksAssociated(ctx context.Context, resourceBinding
 // it returns
 // 1: if we apply the overrides successfully
 // 2: if we actually made any changes on the hub cluster
-func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1beta1.ClusterResourceBinding, existingWorks map[string]*fleetv1beta1.Work, cluster *clusterv1beta1.MemberCluster) (bool, bool, error) {
+func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1beta1.ClusterResourceBinding, existingWorks map[string]*fleetv1beta1.Work, cluster *clusterv1beta1.MemberCluster) (bool, bool, bool, error) {
 	updateAny := atomic.NewBool(false)
+	deletedAny := atomic.NewBool(false)
 	resourceBindingRef := klog.KObj(resourceBinding)
 
 	// Refresh the apply strategy for all existing works.
@@ -435,17 +443,17 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be
 		})
 	}
 	if updateErr := errs.Wait(); updateErr != nil {
-		return false, false, updateErr
+		return false, false, false, updateErr
 	}
 
 	// the hash256 function can handle empty list https://go.dev/play/p/_4HW17fooXM
 	resourceOverrideSnapshotHash, err := resource.HashOf(resourceBinding.Spec.ResourceOverrideSnapshots)
 	if err != nil {
-		return false, false, controller.NewUnexpectedBehaviorError(err)
+		return false, false, false, controller.NewUnexpectedBehaviorError(err)
 	}
 	clusterResourceOverrideSnapshotHash, err := resource.HashOf(resourceBinding.Spec.ClusterResourceOverrideSnapshots)
 	if err != nil {
-		return false, false, controller.NewUnexpectedBehaviorError(err)
+		return false, false, false, controller.NewUnexpectedBehaviorError(err)
 	}
 	// TODO: check all work synced first before fetching the snapshots after we put ParentResourceOverrideSnapshotHashAnnotation and ParentClusterResourceOverrideSnapshotHashAnnotation in all the work objects
 
@@ -456,22 +464,22 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be
 			// the resourceIndex is deleted but the works might still be up to date with the binding.
 			if areAllWorkSynced(existingWorks, resourceBinding, resourceOverrideSnapshotHash, clusterResourceOverrideSnapshotHash) {
 				klog.V(2).InfoS("All the works are synced with the resourceBinding even if the resource snapshot index is removed", "resourceBinding", resourceBindingRef)
-				return true, updateAny.Load(), nil
+				return true, updateAny.Load(), deletedAny.Load(), nil
 			}
-			return false, false, controller.NewUserError(err)
+			return false, false, false, controller.NewUserError(err)
 		}
 		// TODO(RZ): handle errResourceNotFullyCreated error so we don't need to wait for all the snapshots to be created
-		return false, false, err
+		return false, false, false, err
 	}
 
 	croMap, err := r.fetchClusterResourceOverrideSnapshots(ctx, resourceBinding)
 	if err != nil {
-		return false, false, err
+		return false, false, false, err
 	}
 
 	roMap, err := r.fetchResourceOverrideSnapshots(ctx, resourceBinding)
 	if err != nil {
-		return false, false, err
+		return false, false, false, err
 	}
 
 	// issue all the create/update requests for the corresponding works for each snapshot in parallel
@@ -484,15 +492,15 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be
 		workNamePrefix, err := getWorkNamePrefixFromSnapshotName(snapshot)
 		if err != nil {
 			klog.ErrorS(err, "Encountered a mal-formatted resource snapshot", "resourceSnapshot", klog.KObj(snapshot))
-			return false, false, err
+			return false, false, false, err
 		}
 		var simpleManifests []fleetv1beta1.Manifest
 		for j := range snapshot.Spec.SelectedResources {
 			selectedResource := snapshot.Spec.SelectedResources[j].DeepCopy()
 			// TODO: override the content of the wrapped resource instead of the envelope itself
 			resourceDeleted, overrideErr := r.applyOverrides(selectedResource, cluster, croMap, roMap)
 			if overrideErr != nil {
-				return false, false, overrideErr
+				return false, false, false, overrideErr
 			}
 			if resourceDeleted {
 				klog.V(2).InfoS("The resource is deleted by the override rules", "snapshot", klog.KObj(snapshot), "selectedResource", snapshot.Spec.SelectedResources[j])
@@ -503,14 +511,14 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be
 			var uResource unstructured.Unstructured
 			if unMarshallErr := uResource.UnmarshalJSON(selectedResource.Raw); unMarshallErr != nil {
 				klog.ErrorS(unMarshallErr, "work has invalid content", "snapshot", klog.KObj(snapshot), "selectedResource", selectedResource.Raw)
-				return true, false, controller.NewUnexpectedBehaviorError(unMarshallErr)
+				return true, false, false, controller.NewUnexpectedBehaviorError(unMarshallErr)
 			}
 			if uResource.GetObjectKind().GroupVersionKind() == utils.ConfigMapGVK &&
 				len(uResource.GetAnnotations()[fleetv1beta1.EnvelopeConfigMapAnnotation]) != 0 {
 				// get a work object for the enveloped configMap
 				work, err := r.getConfigMapEnvelopWorkObj(ctx, workNamePrefix, resourceBinding, snapshot, &uResource, resourceOverrideSnapshotHash, clusterResourceOverrideSnapshotHash)
 				if err != nil {
-					return true, false, err
+					return true, false, false, err
 				}
 				activeWork[work.Name] = work
 				newWork = append(newWork, work)
@@ -545,6 +553,7 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be
 	}
 
 	//  delete the works that are not associated with any resource snapshot
+	deletedWork := make([]*fleetv1beta1.Work, len(existingWorks))
 	for i := range existingWorks {
 		work := existingWorks[i]
 		if _, exist := activeWork[work.Name]; exist {
@@ -557,18 +566,20 @@ func (r *Reconciler) syncAllWork(ctx context.Context, resourceBinding *fleetv1be
 					return controller.NewAPIServerError(false, err)
 				}
 			}
+			deletedWork = append(deletedWork, work)
 			klog.V(2).InfoS("Deleted the work that is not associated with any resource snapshot", "work", klog.KObj(work))
-			updateAny.Store(true)
+			//updateAny.Store(true)
+			deletedAny.Store(true)
 			return nil
 		})
 	}
 
 	// wait for all the create/update/delete requests to finish
 	if updateErr := errs.Wait(); updateErr != nil {
-		return true, false, updateErr
+		return true, false, false, updateErr
 	}
-	klog.V(2).InfoS("Successfully synced all the work associated with the resourceBinding", "updateAny", updateAny.Load(), "resourceBinding", resourceBindingRef)
-	return true, updateAny.Load(), nil
+	klog.V(2).InfoS("Successfully synced all the work associated with the resourceBinding", "updateAny", updateAny.Load(), "deletedAny", deletedAny.Load(), "resourceBinding", resourceBindingRef)
+	return true, updateAny.Load(), deletedAny.Load(), nil
 }
 
 // syncApplyStrategy syncs the apply strategy specified on a ClusterResourceBinding object
@@ -845,7 +856,7 @@ const (
 )
 
 // setBindingStatus sets the binding status based on the works associated with the binding.
-func setBindingStatus(works map[string]*fleetv1beta1.Work, resourceBinding *fleetv1beta1.ClusterResourceBinding) {
+func setBindingStatus(workDeleted bool, works map[string]*fleetv1beta1.Work, resourceBinding *fleetv1beta1.ClusterResourceBinding) {
 	bindingRef := klog.KObj(resourceBinding)
 
 	// Note (chenyu1): the work generator will refresh the status of a ClusterResourceBinding using
@@ -871,7 +882,7 @@ func setBindingStatus(works map[string]*fleetv1beta1.Work, resourceBinding *flee
 	} else {
 		// Set the Applied and Available condition if (and only if) a ClientSideApply or ServerSideApply
 		// apply strategy is currently being used.
-		appliedSummarizedStatus = setAllWorkAppliedCondition(works, resourceBinding)
+		appliedSummarizedStatus = setAllWorkAppliedCondition(workDeleted, works, resourceBinding)
 		// Note that Fleet will only set the Available condition if the apply op itself is successful, i.e.,
 		// the Applied condition is True.
 		availabilitySummarizedStatus = setAllWorkAvailableCondition(works, resourceBinding)
@@ -1007,7 +1018,7 @@ func setBindingStatus(works map[string]*fleetv1beta1.Work, resourceBinding *flee
 //
 // The Applied condition of a ClusterResourceBinding object is set to True if and only if all the
 // related Work objects have their Applied condition set to True.
-func setAllWorkAppliedCondition(works map[string]*fleetv1beta1.Work, binding *fleetv1beta1.ClusterResourceBinding) workConditionSummarizedStatus {
+func setAllWorkAppliedCondition(workDeleted bool, works map[string]*fleetv1beta1.Work, binding *fleetv1beta1.ClusterResourceBinding) workConditionSummarizedStatus {
 	// Fleet here makes a clear distinction between incomplete, failed, and successful apply operations.
 	// This is to ensure that stale apply information (esp. those set before
 	// an apply strategy change) will not leak into the current apply operations.
@@ -1038,9 +1049,20 @@ func setAllWorkAppliedCondition(works map[string]*fleetv1beta1.Work, binding *fl
 	}
 
 	switch {
+	case workDeleted:
+		// Some work objects are being deleted.
+		klog.V(2).InfoS("Some works have not yet completed the apply op as some are deleting", "binding", klog.KObj(binding))
+		binding.SetConditions(metav1.Condition{
+			Status:             metav1.ConditionFalse,
+			Type:               string(fleetv1beta1.ResourceBindingApplied),
+			Reason:             condition.WorkNotAppliedReason,
+			Message:            "Some work objects have been deleted. Some works have not yet completed the apply op.",
+			ObservedGeneration: binding.GetGeneration(),
+		})
+		return workConditionSummarizedStatusFalse
 	case !areAllWorksApplyOpsCompleted:
 		// Not all Work objects have completed the apply op.
-		klog.V(2).InfoS("Some works are not yet completed the apply op", "binding", klog.KObj(binding), "firstWorkWithIncompleteApplyOp", klog.KObj(firstWorkWithIncompleteApplyOp))
+		klog.V(2).InfoS("Some works have not yet completed the apply op", "binding", klog.KObj(binding), "firstWorkWithIncompleteApplyOp", klog.KObj(firstWorkWithIncompleteApplyOp))
 		binding.SetConditions(metav1.Condition{
 			Status:             metav1.ConditionFalse,
 			Type:               string(fleetv1beta1.ResourceBindingApplied),

pkg/controllers/workgenerator/controller_integration_test.go

@@ -596,7 +596,7 @@ var _ = Describe("Test Work Generator Controller", func() {
 			})
 		})
 
-		Context("Test Bound ClusterResourceBinding with a single resource snapshot with envelop objects", func() {
+		Context("Test Bound ClusterResourceBinding with a single resource snapshot with envelop objects", Serial, func() {
 			var masterSnapshot *placementv1beta1.ClusterResourceSnapshot
 
 			BeforeEach(func() {
@@ -1210,6 +1210,7 @@ var _ = Describe("Test Work Generator Controller", func() {
 					return errors.IsNotFound(err)
 				}, duration, interval).Should(BeTrue(), "controller should remove work in hub cluster that is no longer needed")
 				By(fmt.Sprintf("second work %s is deleted in %s", work.Name, work.Namespace))
+				verifyBindingStatusSyncedNotApplied(binding, false, false)
 			})
 
 			It("Should remove binding after all work associated with deleted bind are deleted", func() {

pkg/controllers/workgenerator/controller_test.go

@@ -528,6 +528,7 @@ func TestUpsertWork(t *testing.T) {
 func TestSetAllWorkAppliedCondition(t *testing.T) {
 	tests := map[string]struct {
 		works                            map[string]*fleetv1beta1.Work
+		workDeleted                      bool
 		generation                       int64
 		wantAppliedCond                  metav1.Condition
 		wantWorkAppliedCondSummaryStatus workConditionSummarizedStatus
@@ -565,7 +566,8 @@ func TestSetAllWorkAppliedCondition(t *testing.T) {
 					},
 				},
 			},
-			generation: 1,
+			workDeleted: false,
+			generation:  1,
 			wantAppliedCond: metav1.Condition{
 				Status:             metav1.ConditionTrue,
 				Type:               string(fleetv1beta1.ResourceBindingApplied),
@@ -607,7 +609,8 @@ func TestSetAllWorkAppliedCondition(t *testing.T) {
 					},
 				},
 			},
-			generation: 1,
+			workDeleted: false,
+			generation:  1,
 			wantAppliedCond: metav1.Condition{
 				Status:             metav1.ConditionFalse,
 				Type:               string(fleetv1beta1.ResourceBindingApplied),
@@ -649,7 +652,8 @@ func TestSetAllWorkAppliedCondition(t *testing.T) {
 					},
 				},
 			},
-			generation: 1,
+			workDeleted: false,
+			generation:  1,
 			wantAppliedCond: metav1.Condition{
 				Status:             metav1.ConditionFalse,
 				Type:               string(fleetv1beta1.ResourceBindingApplied),
@@ -685,7 +689,8 @@ func TestSetAllWorkAppliedCondition(t *testing.T) {
 					},
 				},
 			},
-			generation: 1,
+			workDeleted: false,
+			generation:  1,
 			wantAppliedCond: metav1.Condition{
 				Status:             metav1.ConditionFalse,
 				Type:               string(fleetv1beta1.ResourceBindingApplied),
@@ -694,6 +699,30 @@ func TestSetAllWorkAppliedCondition(t *testing.T) {
 			},
 			wantWorkAppliedCondSummaryStatus: workConditionSummarizedStatusIncomplete,
 		},
+		"one work was deleted": {
+			works: map[string]*fleetv1beta1.Work{
+				// appliedWork1 was deleted
+
+				"notAppliedWork2": {
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "work2",
+						Generation: 123,
+					},
+					Status: fleetv1beta1.WorkStatus{
+						Conditions: []metav1.Condition{},
+					},
+				},
+			},
+			workDeleted: true,
+			generation:  1,
+			wantAppliedCond: metav1.Condition{
+				Status:             metav1.ConditionFalse,
+				Type:               string(fleetv1beta1.ResourceBindingApplied),
+				Reason:             condition.WorkNotAppliedReason,
+				ObservedGeneration: 1,
+			},
+			wantWorkAppliedCondSummaryStatus: workConditionSummarizedStatusFalse,
+		},
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -703,7 +732,7 @@ func TestSetAllWorkAppliedCondition(t *testing.T) {
 					Generation: tt.generation,
 				},
 			}
-			workAppliedCondSummaryStatus := setAllWorkAppliedCondition(tt.works, binding)
+			workAppliedCondSummaryStatus := setAllWorkAppliedCondition(tt.workDeleted, tt.works, binding)
 			if workAppliedCondSummaryStatus != tt.wantWorkAppliedCondSummaryStatus {
 				t.Errorf("setAllWorkAppliedCondition() = %v, want %v", workAppliedCondSummaryStatus, tt.wantWorkAppliedCondSummaryStatus)
 			}
@@ -2372,7 +2401,7 @@ func TestSetBindingStatus(t *testing.T) {
 					ApplyStrategy: tt.applyStrategy,
 				},
 			}
-			setBindingStatus(tt.works, binding)
+			setBindingStatus(false, tt.works, binding)
 			got := binding.Status.FailedPlacements
 			// setBindingStatus is using map to populate the placements.
 			// There is no default order in traversing the map.