fix e2e and integration tests

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

Author: britaniar

pkg/controllers/workapplier/controller.go

@@ -501,7 +501,7 @@ func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv
 			// If the Work object allows co-ownership, we need to change the blockOwnerDeletion to false
 			// so that the appliedWork can be deleted without waiting for the object that has another owner to be deleted.
 			klog.V(2).InfoS("Work object allows co-ownership; updating owner reference to not block deletion", "work", work.Name)
-			if err := r.updateAppliedWorkOwnerReference(ctx, work); !apierrors.IsNotFound(err) && err != nil {
+			if err := r.updateOwnerReference(ctx, work); !apierrors.IsNotFound(err) && err != nil {
 				klog.ErrorS(err, "Failed to update owner reference for the work", "work", work.Name)
 				return ctrl.Result{}, controller.NewAPIServerError(false, err)
 			}
@@ -517,18 +517,18 @@ func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv
 	}
 
 	klog.V(2).InfoS("Deletion for appliedWork is in progress", "appliedWork", work.Name)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: time.Minute}, nil
 }
 
-func (r *Reconciler) updateAppliedWorkOwnerReference(ctx context.Context, work *fleetv1beta1.Work) error {
+func (r *Reconciler) updateOwnerReference(ctx context.Context, work *fleetv1beta1.Work) error {
 	// Get the AppliedWork object
 	var appliedWork fleetv1beta1.AppliedWork
 	if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: work.Name}, &appliedWork); err != nil {
 		if apierrors.IsNotFound(err) {
-			klog.V(2).InfoS("AppliedWork not found for updateAppliedWorkOwnerReference", "appliedWork", work.Name)
+			klog.V(2).InfoS("AppliedWork not found for updateOwnerReference", "appliedWork", work.Name)
 			return err
 		}
-		klog.ErrorS(err, "Failed to get AppliedWork for updateAppliedWorkOwnerReference", "appliedWork", work.Name)
+		klog.ErrorS(err, "Failed to get AppliedWork for updateOwnerReference", "appliedWork", work.Name)
 		return err
 	}
 
@@ -549,36 +549,31 @@ func (r *Reconciler) updateAppliedWorkOwnerReference(ctx context.Context, work *
 			if apierrors.IsNotFound(err) {
 				continue
 			}
-			klog.ErrorS(err, "Failed to get manifest for updateAppliedWorkOwnerReference", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
+			klog.ErrorS(err, "Failed to get manifest for updateOwnerReference", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
 			return err
 		}
-		// Check if there is more than one owner reference
-		if len(obj.GetOwnerReferences()) > 1 {
-			owners := obj.GetOwnerReferences()
-			updated := false
-			for i := range owners {
-				// Check if the owner reference matches the appliedWork UID and has BlockOwnerDeletion set to true
-				if owners[i].UID == appliedWork.UID && owners[i].BlockOwnerDeletion != nil && *owners[i].BlockOwnerDeletion {
-					// If we found an owner reference to update, set BlockOwnerDeletion to false
-					b := false
-					owners[i].BlockOwnerDeletion = &b
-					updated = true
-					break
-				}
+		owners := obj.GetOwnerReferences()
+		// Check if the owner reference already exists
+		updated := false
+		for i := range owners {
+			if owners[i].UID == appliedWork.UID && owners[i].BlockOwnerDeletion != nil && *owners[i].BlockOwnerDeletion {
+				b := false
+				owners[i].BlockOwnerDeletion = &b
+				updated = true
 			}
-			if updated {
-				obj.SetOwnerReferences(owners)
-				if res.Namespace != "" {
-					_, err = r.spokeDynamicClient.Resource(gvr).Namespace(res.Namespace).Update(ctx, obj, metav1.UpdateOptions{})
-				} else {
-					_, err = r.spokeDynamicClient.Resource(gvr).Update(ctx, obj, metav1.UpdateOptions{})
-				}
-				if err != nil {
-					klog.ErrorS(err, "Failed to update manifest owner reference blockOwnerDeletion", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
-					return err
-				} else {
-					klog.InfoS("Set blockOwnerDeletion=false for owner reference", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
-				}
+		}
+		if updated {
+			obj.SetOwnerReferences(owners)
+			if res.Namespace != "" {
+				_, err = r.spokeDynamicClient.Resource(gvr).Namespace(res.Namespace).Update(ctx, obj, metav1.UpdateOptions{})
+			} else {
+				_, err = r.spokeDynamicClient.Resource(gvr).Update(ctx, obj, metav1.UpdateOptions{})
+			}
+			if err != nil {
+				klog.ErrorS(err, "Failed to update manifest owner reference blockOwnerDeletion", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
+				return err
+			} else {
+				klog.InfoS("Set blockOwnerDeletion=false for owner reference", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
 			}
 		}
 	}

pkg/controllers/workapplier/controller_integration_test.go

@@ -1374,12 +1374,6 @@ var _ = Describe("applying manifests", func() {
 		// The environment prepared by the envtest package does not support namespace
 		// deletion; each test case would use a new namespace.
 		nsName := fmt.Sprintf(nsNameTemplate, utils.RandStr())
-		anotherOwnerReference := metav1.OwnerReference{
-			APIVersion: "another-api-version",
-			Kind:       "another-kind",
-			Name:       "another-owner",
-			UID:        "another-uid",
-		}
 
 		var appliedWorkOwnerRef *metav1.OwnerReference
 		var regularNS *corev1.Namespace
@@ -1398,7 +1392,7 @@ var _ = Describe("applying manifests", func() {
 			regularDeployJSON := marshalK8sObjJSON(regularDeploy)
 
 			// Create a new Work object with all the manifest JSONs.
-			createWorkObject(workName, &fleetv1beta1.ApplyStrategy{AllowCoOwnership: true}, regularNSJSON, regularDeployJSON)
+			createWorkObject(workName, nil, regularNSJSON, regularDeployJSON)
 		})
 
 		It("should add cleanup finalizer to the Work object", func() {
@@ -1534,75 +1528,82 @@ var _ = Describe("applying manifests", func() {
 		})
 
 		It("can update object to add another owner reference", func() {
-			// Retrieve the Deployment object.
-			gotDeploy := &appsv1.Deployment{}
-			Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy)).To(Succeed(), "Failed to retrieve the Deployment object")
-
-			// Add another owner reference to the Deployment object.
-			gotDeploy.OwnerReferences = append(gotDeploy.OwnerReferences, anotherOwnerReference)
-			Expect(memberClient.Update(ctx, gotDeploy)).To(Succeed(), "Failed to update the Deployment object with another owner reference")
-
-			// Ensure that the Deployment object has been updated as expected.
-			Eventually(func() error {
-				// Retrieve the Deployment object again.
-				if err := memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy); err != nil {
-					return fmt.Errorf("failed to retrieve the Deployment object: %w", err)
-				}
-
-				// Check that the Deployment object has been updated as expected.
-				if len(gotDeploy.OwnerReferences) != 2 {
-					return fmt.Errorf("expected 2 owner references, got %d", len(gotDeploy.OwnerReferences))
-				}
-				for _, ownerRef := range gotDeploy.OwnerReferences {
-					if ownerRef.UID == anotherOwnerReference.UID {
-						return nil
-					}
-				}
-				return fmt.Errorf("another owner reference not found in the Deployment object")
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to find another owner reference on Deployment")
 		})
 
-		It("should start deleting the Work object", func() {
-			// Start deleting the Work object.
-			deleteWorkObject(workName)
+		It("should garbage collect removed manifests", func() {
+			deployRemovedActual := regularDeployRemovedActual(nsName, deployName)
+			Eventually(deployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object")
 		})
 
-		It("should start deleting the AppliedWork object", func() {
-			// Ensure that the Work object is being deleted.
-			Eventually(func() error {
-				appliedWork := &fleetv1beta1.AppliedWork{}
-				if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); err != nil {
-					return err
-				}
-				if !appliedWork.DeletionTimestamp.IsZero() {
-					return fmt.Errorf("appliedWork object still is not being deleted")
-				}
-				return nil
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object")
+		It("should update the Work object status", func() {
+			// Prepare the status information.
+			workConds := []metav1.Condition{
+				{
+					Type:   fleetv1beta1.WorkConditionTypeApplied,
+					Status: metav1.ConditionTrue,
+					Reason: WorkAllManifestsAppliedReason,
+				},
+				{
+					Type:   fleetv1beta1.WorkConditionTypeAvailable,
+					Status: metav1.ConditionTrue,
+					Reason: WorkAllManifestsAvailableReason,
+				},
+			}
+			manifestConds := []fleetv1beta1.ManifestCondition{
+				{
+					Identifier: fleetv1beta1.WorkResourceIdentifier{
+						Ordinal:  0,
+						Group:    "",
+						Version:  "v1",
+						Kind:     "Namespace",
+						Resource: "namespaces",
+						Name:     nsName,
+					},
+					Conditions: []metav1.Condition{
+						{
+							Type:               fleetv1beta1.WorkConditionTypeApplied,
+							Status:             metav1.ConditionTrue,
+							Reason:             string(ManifestProcessingApplyResultTypeApplied),
+							ObservedGeneration: 0,
+						},
+						{
+							Type:               fleetv1beta1.WorkConditionTypeAvailable,
+							Status:             metav1.ConditionTrue,
+							Reason:             string(ManifestProcessingAvailabilityResultTypeAvailable),
+							ObservedGeneration: 0,
+						},
+					},
+				},
+			}
+
+			workStatusUpdatedActual := workStatusUpdated(workName, workConds, manifestConds, nil, nil)
+			Eventually(workStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update work status")
 		})
 
-		It("should update owner reference from the Deployment object", func() {
-			// Ensure that the Deployment object has been updated to remove the owner reference.
-			Eventually(func() error {
-				// Retrieve the Deployment object again.
-				gotDeploy := &appsv1.Deployment{}
-				if err := memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy); err != nil {
-					return fmt.Errorf("failed to retrieve the Deployment object: %w", err)
-				}
+		It("should update the AppliedWork object status", func() {
+			// Prepare the status information.
+			appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{
+				{
+					WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{
+						Ordinal:  0,
+						Group:    "",
+						Version:  "v1",
+						Kind:     "Namespace",
+						Resource: "namespaces",
+						Name:     nsName,
+					},
+					UID: regularNS.UID,
+				},
+			}
 
-				// Check that the Deployment object has been updated as expected.
-				for _, ownerRef := range gotDeploy.OwnerReferences {
-					if ownerRef.UID == appliedWorkOwnerRef.UID {
-						if *ownerRef.BlockOwnerDeletion {
-							return fmt.Errorf("owner reference from AppliedWork still has BlockOwnerDeletion set to true")
-						}
-					}
-				}
-				return nil
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment")
+			appliedWorkStatusUpdatedActual := appliedWorkStatusUpdated(workName, appliedResourceMeta)
+			Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status")
 		})
 
 		AfterAll(func() {
+			// Delete the Work object and related resources.
+			deleteWorkObject(workName)
+
 			// Ensure that the AppliedWork object has been removed.
 			appliedWorkRemovedActual := appliedWorkRemovedActual(workName)
 			Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object")

test/e2e/enveloped_object_placement_test.go

@@ -207,6 +207,7 @@ var _ = Describe("placing wrapped resources using a CRP", func() {
 		AfterAll(func() {
 			By(fmt.Sprintf("deleting envelope %s", testResourceEnvelope.Name))
 			Expect(hubClient.Delete(ctx, &testResourceEnvelope)).To(Succeed(), "Failed to delete ResourceEnvelope")
+			By(fmt.Sprintf("deleting envelope %s", testClusterResourceEnvelope.Name))
 			Expect(hubClient.Delete(ctx, &testClusterResourceEnvelope)).To(Succeed(), "Failed to delete testClusterResourceEnvelope")
 			By(fmt.Sprintf("deleting placement %s and related resources", crpName))
 			ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

test/e2e/rollout_test.go

@@ -718,8 +718,8 @@ var _ = Describe("placing wrapped resources using a CRP", Ordered, func() {
 		})
 		// job is not trackable, so we need to wait for a bit longer for each roll out
 		It("should update CRP status as expected", func() {
-			crpStatusUpdatedActual := customizedCRPStatusUpdatedActual(crpName, wantSelectedResources, allMemberClusterNames, nil, "0", false)
-			Eventually(crpStatusUpdatedActual, 6*time.Duration(unAvailablePeriodSeconds)*time.Second, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+			crpStatusUpdatedActual := customizedCRPStatusUpdatedActual(crpName, wantSelectedResources, allMemberClusterNames, nil, "1", false)
+			Eventually(crpStatusUpdatedActual, 5*time.Duration(unAvailablePeriodSeconds)*time.Second, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
 		})
 
 		AfterAll(func() {

test/e2e/utils_test.go

@@ -1234,6 +1234,7 @@ func readJobTestManifest(testManifest *batchv1.Job) {
 
 func readEnvelopeResourceTestManifest(testEnvelopeObj *placementv1beta1.ResourceEnvelope) {
 	By("Read testEnvelopConfigMap resource")
+	testEnvelopeObj.ResourceVersion = ""
 	err := utils.GetObjectFromManifest("resources/test-envelope-object.yaml", testEnvelopeObj)
 	Expect(err).Should(Succeed())
 }