From 65f17bf37f4b67968259cd0647afff5cb1a8478c Mon Sep 17 00:00:00 2001 From: Tianxin Dong Date: Mon, 30 May 2022 19:27:43 +0800 Subject: [PATCH] Fix: fix the dependency gc policy to reverse dependency (#4063) Signed-off-by: FogDong --- .../gc-policy/reverse-dependency.md | 4 +- .../v1alpha2/application/gc_policy_test.go | 10 ++- pkg/resourcekeeper/gc.go | 25 +++--- pkg/resourcekeeper/gc_test.go | 83 +++++++++++++++++++ 4 files changed, 105 insertions(+), 17 deletions(-) diff --git a/docs/examples/app-with-policy/gc-policy/reverse-dependency.md b/docs/examples/app-with-policy/gc-policy/reverse-dependency.md index b094a1487..dd086ac94 100644 --- a/docs/examples/app-with-policy/gc-policy/reverse-dependency.md +++ b/docs/examples/app-with-policy/gc-policy/reverse-dependency.md @@ -1,6 +1,6 @@ # How to garbage collect resources in the order of dependency -If you want to garbage collect resources in the order of dependency, you can add `order: dependency` in the `garbage-collect` policy. +If you want to garbage collect resources in the order of reverse dependency, you can add `order: dependency` in the `garbage-collect` policy. > Notice that this order policy is only valid for the resources that are created in the components. @@ -8,7 +8,7 @@ In the following example, component `test1` depends on `test2`, and `test2` need So the order of deployment is: `test3 -> test2 -> test1`. -When we add `order: dependency` in `garbage-collect` policy and delete the application, the order of garbage collect is: `test3 -> test2 -> test1`. +When we add `order: dependency` in `garbage-collect` policy and delete the application, the order of garbage collect is: `test1 -> test2 -> test3`. ```yaml apiVersion: core.oam.dev/v1beta1 diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/gc_policy_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/gc_policy_test.go index 882cdfe55..20db28087 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/gc_policy_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/gc_policy_test.go @@ -561,15 +561,21 @@ var _ = Describe("Test Application with GC options", func() { By("delete application") Expect(k8sClient.Delete(ctx, app)).Should(BeNil()) - By("worker3 will be deleted") + By("worker1 will be deleted") testutil.ReconcileOnce(reconciler, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(app)}) Expect(k8sClient.List(ctx, workerList, listOpts...)).Should(BeNil()) + for _, worker := range workerList.Items { + Expect(worker.Name).ShouldNot(Equal("worker1")) + } Expect(len(workerList.Items)).Should(Equal(2)) By("worker2 will be deleted") testutil.ReconcileOnce(reconciler, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(app)}) Expect(k8sClient.List(ctx, workerList, listOpts...)).Should(BeNil()) Expect(len(workerList.Items)).Should(Equal(1)) - By("worker1 will be deleted") + for _, worker := range workerList.Items { + Expect(worker.Name).ShouldNot(Equal("worker2")) + } + By("worker3 will be deleted") testutil.ReconcileOnce(reconciler, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(app)}) Expect(k8sClient.List(ctx, workerList, listOpts...)).Should(BeNil()) Expect(len(workerList.Items)).Should(Equal(0)) diff --git a/pkg/resourcekeeper/gc.go b/pkg/resourcekeeper/gc.go index 4a2d0c219..4cbabfaf0 100644 --- a/pkg/resourcekeeper/gc.go +++ b/pkg/resourcekeeper/gc.go @@ -327,26 +327,25 @@ func (h *gcHandler) deleteManagedResource(ctx context.Context, mr v1beta1.Manage func (h *gcHandler) checkDependentComponent(mr v1beta1.ManagedResource) []string { dependent := make([]string, 0) - inputs := make([]string, 0) + outputs := make([]string, 0) for _, comp := range h.app.Spec.Components { if comp.Name == mr.Component { - dependent = comp.DependsOn - if len(comp.Inputs) > 0 { - for _, input := range comp.Inputs { - inputs = append(inputs, input.From) + for _, output := range comp.Outputs { + outputs = append(outputs, output.Name) + } + } else { + for _, dependsOn := range comp.DependsOn { + if dependsOn == mr.Component { + dependent = append(dependent, comp.Name) + break } - } else { - return dependent } - break } } for _, comp := range h.app.Spec.Components { - if len(comp.Outputs) > 0 { - for _, output := range comp.Outputs { - if utils.StringsContain(inputs, output.Name) { - dependent = append(dependent, comp.Name) - } + for _, input := range comp.Inputs { + if utils.StringsContain(outputs, input.From) { + dependent = append(dependent, comp.Name) } } } diff --git a/pkg/resourcekeeper/gc_test.go b/pkg/resourcekeeper/gc_test.go index dbcf9973e..abe7258b8 100644 --- a/pkg/resourcekeeper/gc_test.go +++ b/pkg/resourcekeeper/gc_test.go @@ -253,3 +253,86 @@ func TestResourceKeeperGarbageCollect(t *testing.T) { r.NoError(err) r.True(finished) } + +func TestCheckDependentComponent(t *testing.T) { + rk := &resourceKeeper{ + app: &v1beta1.Application{ + Spec: v1beta1.ApplicationSpec{ + Components: []apicommon.ApplicationComponent{ + { + Name: "comp-1", + Outputs: apicommon.StepOutputs{ + { + Name: "output-1", + }, + }, + }, + { + Name: "comp-2", + Outputs: apicommon.StepOutputs{ + { + Name: "output-2", + }, + }, + }, + { + Name: "comp-3", + Inputs: apicommon.StepInputs{ + { + From: "output-1", + }, + { + From: "output-2", + }, + }, + }, + { + Name: "comp-4", + DependsOn: []string{"comp-3"}, + }, + { + Name: "comp-5", + DependsOn: []string{"comp-4", "comp-3"}, + }, + }, + }, + }, + } + testCases := []struct { + comp string + result []string + }{ + { + comp: "comp-1", + result: []string{"comp-3"}, + }, + { + comp: "comp-2", + result: []string{"comp-3"}, + }, + { + comp: "comp-3", + result: []string{"comp-4", "comp-5"}, + }, + { + comp: "comp-4", + result: []string{"comp-5"}, + }, + { + comp: "comp-5", + result: []string{}, + }, + } + gcHandler := &gcHandler{ + resourceKeeper: rk, + } + r := require.New(t) + for _, tc := range testCases { + mr := v1beta1.ManagedResource{ + OAMObjectReference: apicommon.OAMObjectReference{ + Component: tc.comp, + }, + } + r.Equal(gcHandler.checkDependentComponent(mr), tc.result) + } +}