From 61c038e3f35a770cc4736689a9a491f4b0fcb5ed Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Fri, 25 Feb 2022 15:52:47 +0800 Subject: [PATCH] some code refactor (#118) 1. use resourceIdentifier to track resource 2. pass context for deleteAppliedResource Signed-off-by: Jian Qiu --- pkg/helper/helper_test.go | 2 +- pkg/helper/helpers.go | 5 +++-- .../appliedmanifestwork_controller.go | 12 ++++-------- .../appliedmanifestwork_finalize_controller.go | 6 +++--- .../manifestwork_finalize_controller.go | 2 +- 5 files changed, 12 insertions(+), 15 deletions(-) diff --git a/pkg/helper/helper_test.go b/pkg/helper/helper_test.go index 47f9cb6e3..f7e0569a2 100644 --- a/pkg/helper/helper_test.go +++ b/pkg/helper/helper_test.go @@ -417,7 +417,7 @@ func TestDeleteAppliedResourcess(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { fakeDynamicClient := fakedynamic.NewSimpleDynamicClient(scheme, c.existingResources...) - actual, err := DeleteAppliedResources(c.resourcesToRemove, "testing", fakeDynamicClient, eventstesting.NewTestingEventRecorder(t), c.owner) + actual, err := DeleteAppliedResources(context.TODO(), c.resourcesToRemove, "testing", fakeDynamicClient, eventstesting.NewTestingEventRecorder(t), c.owner) if err != nil { t.Errorf("unexpected err: %v", err) } diff --git a/pkg/helper/helpers.go b/pkg/helper/helpers.go index 9b21d32c8..82d6090e4 100644 --- a/pkg/helper/helpers.go +++ b/pkg/helper/helpers.go @@ -192,6 +192,7 @@ func updateManifestWorkStatus( // DeleteAppliedResources deletes all given applied resources and returns those pending for finalization // If the uid recorded in resources is different from what we get by client, ignore the deletion. func DeleteAppliedResources( + ctx context.Context, resources []workapiv1.AppliedManifestResourceMeta, reason string, dynamicClient dynamic.Interface, @@ -214,7 +215,7 @@ func DeleteAppliedResources( u, err := dynamicClient. Resource(gvr). Namespace(resource.Namespace). - Get(context.TODO(), resource.Name, metav1.GetOptions{}) + Get(ctx, resource.Name, metav1.GetOptions{}) if errors.IsNotFound(err) { klog.V(2).Infof("Resource %v with key %s/%s is removed Successfully", gvr, resource.Namespace, resource.Name) continue @@ -245,7 +246,7 @@ func DeleteAppliedResources( } u.SetOwnerReferences(existingOwner) - _, err = dynamicClient.Resource(gvr).Namespace(resource.Namespace).Update(context.TODO(), u, metav1.UpdateOptions{}) + _, err = dynamicClient.Resource(gvr).Namespace(resource.Namespace).Update(ctx, u, metav1.UpdateOptions{}) if err != nil { errs = append(errs, fmt.Errorf( "Failed to remove owner from resource %v with key %s/%s: %w", diff --git a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go index 7226e5da2..5142a63a8 100644 --- a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go +++ b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go @@ -163,7 +163,7 @@ func (m *AppliedManifestWorkController) syncManifestWork( reason := fmt.Sprintf("it is no longer maintained by manifestwork %s", manifestWork.Name) resourcesPendingFinalization, errs := helper.DeleteAppliedResources( - noLongerMaintainedResources, reason, m.spokeDynamicClient, controllerContext.Recorder(), *owner) + ctx, noLongerMaintainedResources, reason, m.spokeDynamicClient, controllerContext.Recorder(), *owner) if len(errs) != 0 { return utilerrors.NewAggregate(errs) } @@ -218,17 +218,13 @@ func (m *AppliedManifestWorkController) syncManifestWork( func findUntrackedResources(appliedResources, newAppliedResources []workapiv1.AppliedManifestResourceMeta) []workapiv1.AppliedManifestResourceMeta { var untracked []workapiv1.AppliedManifestResourceMeta - resourceIndex := map[workapiv1.AppliedManifestResourceMeta]struct{}{} + resourceIndex := map[workapiv1.ResourceIdentifier]struct{}{} for _, resource := range newAppliedResources { - key := resource.DeepCopy() - key.UID, key.Version = "", "" - resourceIndex[*key] = struct{}{} + resourceIndex[resource.ResourceIdentifier] = struct{}{} } for _, resource := range appliedResources { - key := resource.DeepCopy() - key.UID, key.Version = "", "" - if _, ok := resourceIndex[*key]; !ok { + if _, ok := resourceIndex[resource.ResourceIdentifier]; !ok { untracked = append(untracked, resource) } } diff --git a/pkg/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go b/pkg/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go index bd29e9068..d37e5a031 100644 --- a/pkg/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go +++ b/pkg/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go @@ -100,7 +100,7 @@ func (m *AppliedManifestWorkFinalizeController) syncAppliedManifestWork(ctx cont // scoped resource correctly. reason := fmt.Sprintf("manifestwork %s is terminating", appliedManifestWork.Spec.ManifestWorkName) resourcesPendingFinalization, errs := helper.DeleteAppliedResources( - appliedManifestWork.Status.AppliedResources, reason, m.spokeDynamicClient, controllerContext.Recorder(), *owner) + ctx, appliedManifestWork.Status.AppliedResources, reason, m.spokeDynamicClient, controllerContext.Recorder(), *owner) updatedAppliedManifestWork := false if len(appliedManifestWork.Status.AppliedResources) != len(resourcesPendingFinalization) { @@ -109,7 +109,7 @@ func (m *AppliedManifestWorkFinalizeController) syncAppliedManifestWork(ctx cont appliedManifestWork, err = m.appliedManifestWorkClient.UpdateStatus(ctx, appliedManifestWork, metav1.UpdateOptions{}) if err != nil { errs = append(errs, fmt.Errorf( - "Failed to update status of AppliedManifestWork %s: %w", originalManifestWork.Name, err)) + "failed to update status of AppliedManifestWork %s: %w", originalManifestWork.Name, err)) } else { updatedAppliedManifestWork = true } @@ -132,7 +132,7 @@ func (m *AppliedManifestWorkFinalizeController) syncAppliedManifestWork(ctx cont helper.RemoveFinalizer(appliedManifestWork, controllers.AppliedManifestWorkFinalizer) _, err = m.appliedManifestWorkClient.Update(ctx, appliedManifestWork, metav1.UpdateOptions{}) if err != nil { - return fmt.Errorf("Failed to remove finalizer from AppliedManifestWork %s: %w", appliedManifestWork.Name, err) + return fmt.Errorf("failed to remove finalizer from AppliedManifestWork %s: %w", appliedManifestWork.Name, err) } return nil } diff --git a/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go b/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go index 2b23caefe..d6943dc6b 100644 --- a/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go +++ b/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go @@ -107,7 +107,7 @@ func (m *ManifestWorkFinalizeController) sync(ctx context.Context, controllerCon helper.RemoveFinalizer(manifestWork, controllers.ManifestWorkFinalizer) _, err = m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{}) if err != nil { - return fmt.Errorf("Failed to remove finalizer from ManifestWork %s/%s: %w", manifestWork.Namespace, manifestWork.Name, err) + return fmt.Errorf("failed to remove finalizer from ManifestWork %s/%s: %w", manifestWork.Namespace, manifestWork.Name, err) } return nil