fix: delete associated works when bindings are deleted (Azure#503)

pkg/controllers/workgenerator/controller.go

@@ -159,6 +159,19 @@ func (r *Reconciler) handleDelete(ctx context.Context, resourceBinding *fleetv1b
 	if err != nil {
 		return ctrl.Result{}, err
 	}
+
+	// delete all the listed works
+	//
+	// TO-DO: this controller should be able to garbage collect all works automatically via
+	// background/foreground cascade deletion. This may render the finalizer unnecessary.
+	for workName := range works {
+		work := works[workName]
+
+		if err := r.Client.Delete(ctx, work); err != nil && !apierrors.IsNotFound(err) {
+			return ctrl.Result{}, controller.NewAPIServerError(false, err)
+		}
+	}
+
 	// remove the work finalizer on the binding if all the work objects are deleted
 	if len(works) == 0 {
 		controllerutil.RemoveFinalizer(resourceBinding, fleetv1beta1.WorkFinalizer)

pkg/controllers/workgenerator/controller_integration_test.go

@@ -37,7 +37,7 @@ var (
 	ignoreConditionOption = cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime", "Message")
 )
 
-var _ = Describe("Test clusterSchedulingPolicySnapshot Controller", func() {
+var _ = Describe("Test Work Generator Controller", func() {
 	const (
 		timeout  = time.Second * 6
 		duration = time.Second * 20
@@ -676,20 +676,30 @@ var _ = Describe("Test clusterSchedulingPolicySnapshot Controller", func() {
 				Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed())
 				Expect(k8sClient.Delete(ctx, binding)).Should(Succeed())
 				By(fmt.Sprintf("resource binding  %s is deleted", binding.Name))
-				// Check the binding is not removed
-				Consistently(func() error {
-					return k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)
-				}, duration, interval).Should(Succeed(), "the binding should not be removed until all the work is removed")
-				// delete the work (as the testEnv doesn't run GC controller)
-				Expect(k8sClient.Delete(ctx, &work)).Should(Succeed())
-				By(fmt.Sprintf("work  %s is deleted", work.Name))
-				// Check the binding is not still removed
-				Consistently(func() error {
-					return k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)
-				}, duration, interval).Should(Succeed(), "the binding should not be removed until all the work is removed")
-				// delete the work2 (as the testEnv doesn't run GC controller)
-				Expect(k8sClient.Delete(ctx, &work2)).Should(Succeed())
-				By(fmt.Sprintf("work2  %s is deleted", work2.Name))
+
+				// verify that all associated works have been deleted
+				Eventually(func() error {
+					workKey1 := types.NamespacedName{
+						Namespace: namespaceName,
+						Name:      fmt.Sprintf(fleetv1beta1.FirstWorkNameFmt, testCRPName),
+					}
+					work1 := v1alpha1.Work{}
+					if err := k8sClient.Get(ctx, workKey1, &work1); !apierrors.IsNotFound(err) {
+						return fmt.Errorf("Work %v still exists or an unexpected error has occurred", workKey1)
+					}
+
+					workKey2 := types.NamespacedName{
+						Namespace: namespaceName,
+						Name:      fmt.Sprintf(fleetv1beta1.WorkNameWithSubindexFmt, testCRPName, 1),
+					}
+					work2 := v1alpha1.Work{}
+					if err := k8sClient.Get(ctx, workKey2, &work2); !apierrors.IsNotFound(err) {
+						return fmt.Errorf("Work %v still exists or an unexpected error has occurred", workKey2)
+					}
+
+					return nil
+				}, timeout, interval).Should(Succeed(), "Failed to clean out all the works")
+
 				// check the binding is deleted
 				Eventually(func() bool {
 					err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)

test/scheduler/pickall_integration_test.go

@@ -0,0 +1,9 @@
+/*
+Copyright (c) Microsoft Corporation.
+Licensed under the MIT license.
+*/
+
+package tests
+
+// This test suite features a number of test cases which cover the scheduler workflow of processing
+// CRPs of the PickAll placement type.