Skip to content

Commit dca21c5

Browse files
britaniarbritaniar
committed
address comments (fix requeue, formatting, etc.)
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
1 parent 44b3a02 commit dca21c5

File tree

5 files changed

+56
-47
lines changed

5 files changed

+56
-47
lines changed

‎pkg/controllers/workapplier/controller.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
66
You may obtain a copy of the License at
77
8-
http://www.apache.org/licenses/LICENSE-2.0
8+
http://www.apache.org/licenses/LICENSE-2.0
99
1010
Unless required by applicable law or agreed to in writing, software
1111
distributed under the License is distributed on an "AS IS" BASIS,
@@ -488,7 +488,7 @@ func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv
488488
return r.removeWorkFinalizer(ctx, work)
489489
}
490490
klog.ErrorS(err, "Failed to get AppliedWork", "appliedWork", work.Name)
491-
return ctrl.Result{}, controller.NewAPIServerError(false, err)
491+
return ctrl.Result{Requeue: true}, controller.NewAPIServerError(false, err)
492492
}
493493

494494
// Delete the appliedWork object.
@@ -555,7 +555,8 @@ func (r *Reconciler) updateOwnerReference(ctx context.Context, appliedWork *flee
555555
if ownerRefs[idx].UID == appliedWorkOwnerRef.UID &&
556556
ownerRefs[idx].Name == appliedWorkOwnerRef.Name &&
557557
ownerRefs[idx].Kind == appliedWorkOwnerRef.Kind &&
558-
ownerRefs[idx].APIVersion == appliedWorkOwnerRef.APIVersion {
558+
ownerRefs[idx].APIVersion == appliedWorkOwnerRef.APIVersion &&
559+
*ownerRefs[idx].BlockOwnerDeletion {
559560
ownerRefs[idx].BlockOwnerDeletion = ptr.To(false)
560561
updated = true
561562
}
@@ -636,7 +637,6 @@ func (r *Reconciler) ensureAppliedWork(ctx context.Context, work *fleetv1beta1.W
636637
return nil, controller.NewAPIServerError(false, err)
637638
}
638639
}
639-
640640
klog.InfoS("Recreated the appliedWork resource", "appliedWork", workRef.Name)
641641
return appliedWork, nil
642642
}

‎pkg/controllers/workapplier/controller_integration_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
66
You may obtain a copy of the License at
77
8-
http://www.apache.org/licenses/LICENSE-2.0
8+
http://www.apache.org/licenses/LICENSE-2.0
99
1010
Unless required by applicable law or agreed to in writing, software
1111
distributed under the License is distributed on an "AS IS" BASIS,

‎pkg/controllers/workapplier/preprocess.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -560,12 +560,14 @@ func removeOwnerRef(obj *unstructured.Unstructured, expectedAppliedWorkOwnerRef
560560

561561
// Re-build the owner references; remove the given one from the list.
562562
for idx := range ownerRefs {
563-
if ownerRefs[idx].UID != expectedAppliedWorkOwnerRef.UID &&
564-
ownerRefs[idx].Name != expectedAppliedWorkOwnerRef.Name &&
565-
ownerRefs[idx].Kind != expectedAppliedWorkOwnerRef.Kind &&
566-
ownerRefs[idx].APIVersion != expectedAppliedWorkOwnerRef.APIVersion {
567-
updatedOwnerRefs = append(updatedOwnerRefs, ownerRefs[idx])
563+
if ownerRefs[idx].UID == expectedAppliedWorkOwnerRef.UID &&
564+
ownerRefs[idx].Name == expectedAppliedWorkOwnerRef.Name &&
565+
ownerRefs[idx].Kind == expectedAppliedWorkOwnerRef.Kind &&
566+
ownerRefs[idx].APIVersion == expectedAppliedWorkOwnerRef.APIVersion {
567+
// Skip the expected owner reference.
568+
continue
568569
}
570+
updatedOwnerRefs = append(updatedOwnerRefs, ownerRefs[idx])
569571
}
570572
obj.SetOwnerReferences(updatedOwnerRefs)
571573
}

‎test/e2e/placement_apply_strategy_test.go‎

Lines changed: 18 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
. "github.com/onsi/ginkgo/v2"
2424
. "github.com/onsi/gomega"
2525
corev1 "k8s.io/api/core/v1"
26-
"k8s.io/apimachinery/pkg/api/errors"
2726
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2827
"k8s.io/apimachinery/pkg/types"
2928
"k8s.io/apimachinery/pkg/util/intstr"
@@ -84,16 +83,6 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
8483

8584
By("deleting created work resources on member cluster")
8685
cleanWorkResourcesOnCluster(allMemberClusters[0])
87-
88-
// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
89-
// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
90-
// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
91-
// and flakiness in subsequent tests.
92-
By("Check if work is deleted")
93-
Eventually(func() bool {
94-
work := &placementv1beta1.Work{}
95-
return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
96-
}, 6*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
9786
})
9887

9988
It("should update CRP status as expected", func() {
@@ -137,8 +126,12 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
137126

138127
if len(ns.OwnerReferences) > 0 {
139128
for _, ownerRef := range ns.OwnerReferences {
140-
if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() && ownerRef.Kind == placementv1beta1.AppliedWorkKind && ownerRef.Name == fmt.Sprintf("%s-work", crpName) && *ownerRef.BlockOwnerDeletion {
141-
return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", workNamespaceName)
129+
if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() &&
130+
ownerRef.Kind == placementv1beta1.AppliedWorkKind &&
131+
ownerRef.Name == fmt.Sprintf("%s-work", crpName) {
132+
if *ownerRef.BlockOwnerDeletion {
133+
return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", workNamespaceName)
134+
}
142135
}
143136
}
144137
}
@@ -273,16 +266,6 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
273266

274267
By("deleting created work resources on member cluster")
275268
cleanWorkResourcesOnCluster(allMemberClusters[0])
276-
277-
// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
278-
// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
279-
// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
280-
// and flakiness in subsequent tests.
281-
By("Check if work is deleted")
282-
Eventually(func() bool {
283-
work := &placementv1beta1.Work{}
284-
return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
285-
}, 6*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
286269
})
287270

288271
It("should update CRP status as expected", func() {
@@ -382,8 +365,12 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
382365

383366
if len(ns.OwnerReferences) > 0 {
384367
for _, ownerRef := range ns.OwnerReferences {
385-
if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() && ownerRef.Kind == placementv1beta1.AppliedWorkKind && ownerRef.Name == fmt.Sprintf("%s-work", crpName) && *ownerRef.BlockOwnerDeletion {
386-
return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", workNamespaceName)
368+
if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() &&
369+
ownerRef.Kind == placementv1beta1.AppliedWorkKind &&
370+
ownerRef.Name == fmt.Sprintf("%s-work", crpName) {
371+
if *ownerRef.BlockOwnerDeletion {
372+
return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", workNamespaceName)
373+
}
387374
}
388375
}
389376
}
@@ -427,16 +414,6 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
427414

428415
By("deleting created work resources on member cluster")
429416
cleanWorkResourcesOnCluster(allMemberClusters[0])
430-
431-
// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
432-
// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
433-
// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
434-
// and flakiness in subsequent tests.
435-
By("Check if work is deleted")
436-
Eventually(func() bool {
437-
work := &placementv1beta1.Work{}
438-
return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
439-
}, 7*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
440417
})
441418

442419
It("should update CRP status as expected", func() {
@@ -482,8 +459,12 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
482459

483460
if len(ns.OwnerReferences) > 0 {
484461
for _, ownerRef := range ns.OwnerReferences {
485-
if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() && ownerRef.Kind == placementv1beta1.AppliedWorkKind && ownerRef.Name == fmt.Sprintf("%s-work", crpName) && *ownerRef.BlockOwnerDeletion {
486-
return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", workNamespaceName)
462+
if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() &&
463+
ownerRef.Kind == placementv1beta1.AppliedWorkKind &&
464+
ownerRef.Name == fmt.Sprintf("%s-work", crpName) {
465+
if *ownerRef.BlockOwnerDeletion {
466+
return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", workNamespaceName)
467+
}
487468
}
488469
}
489470
}

‎test/e2e/utils_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,32 @@ func cleanupCRP(name string) {
815815
// Wait until the CRP is removed.
816816
removedActual := crpRemovedActual(name)
817817
Eventually(removedActual, workloadEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove CRP %s", name)
818+
819+
// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
820+
// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
821+
// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
822+
// and flakiness in subsequent tests.
823+
By("Check if work is deleted")
824+
var workNS string
825+
work := &placementv1beta1.Work{
826+
ObjectMeta: metav1.ObjectMeta{
827+
Name: fmt.Sprintf("%s-work", name),
828+
},
829+
}
830+
Eventually(func() bool {
831+
for i := range allMemberClusters {
832+
workNS = fmt.Sprintf("fleet-member-%s", allMemberClusterNames[i])
833+
if err := hubClient.Get(ctx, types.NamespacedName{Name: work.Name, Namespace: workNS}, work); err != nil {
834+
if k8serrors.IsNotFound(err) {
835+
// Work resource is not found, which is expected.
836+
continue
837+
}
838+
// Some other error occurred, return false to retry.
839+
return false
840+
}
841+
}
842+
return true
843+
}, workloadEventuallyDuration, eventuallyInterval).Should(BeTrue(), fmt.Sprintf("Work resource %s from namespace %s should be deleted from hub", fmt.Sprintf("%s-work", name)))
818844
}
819845

820846
// createResourceOverrides creates a number of resource overrides.

0 commit comments

Comments
 (0)