From c0144865d2f3b2ca735dceaca900b95ccc3fedd8 Mon Sep 17 00:00:00 2001 From: Yue Wang Date: Mon, 26 Jul 2021 14:20:22 +0800 Subject: [PATCH] fix app finalizer bug (#1962) Signed-off-by: roy wang --- .../application/application_controller.go | 32 ++------- .../application/application_finalizer_test.go | 69 +++++++++++++++++++ 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go index 02b38645f..416071a0c 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go @@ -38,7 +38,6 @@ import ( velatypes "github.com/oam-dev/kubevela/apis/types" "github.com/oam-dev/kubevela/pkg/appfile" core "github.com/oam-dev/kubevela/pkg/controller/core.oam.dev" - "github.com/oam-dev/kubevela/pkg/controller/core.oam.dev/v1alpha2/application/dispatch" "github.com/oam-dev/kubevela/pkg/controller/utils" "github.com/oam-dev/kubevela/pkg/cue/packages" "github.com/oam-dev/kubevela/pkg/oam" @@ -59,9 +58,9 @@ const ( legacyResourceTrackerFinalizer = "resourceTracker.finalizer.core.oam.dev" // resourceTrackerFinalizer is to delete the resource tracker of the latest app revision. resourceTrackerFinalizer = "app.oam.dev/resource-tracker-finalizer" - // onlyRevisionFinalizer is to delete all resource trackers of app revisions which may be used + // legacyOnlyRevisionFinalizer is to delete all resource trackers of app revisions which may be used // out of the domain of app controller, e.g., AppRollout controller. - onlyRevisionFinalizer = "app.oam.dev/only-revision-finalizer" + legacyOnlyRevisionFinalizer = "app.oam.dev/only-revision-finalizer" ) // Reconciler reconciles a Application object @@ -253,14 +252,6 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat klog.InfoS("Register new finalizer for application", "application", klog.KObj(app), "finalizer", resourceTrackerFinalizer) return true, errors.Wrap(r.Client.Update(ctx, app), errUpdateApplicationFinalizer) } - if appWillRollout(app) { - klog.InfoS("Found an application which will be released by rollout", "application", klog.KObj(app)) - if !meta.FinalizerExists(app, onlyRevisionFinalizer) { - meta.AddFinalizer(app, onlyRevisionFinalizer) - klog.InfoS("Register new finalizer for application", "application", klog.KObj(app), "finalizer", onlyRevisionFinalizer) - return true, errors.Wrap(r.Client.Update(ctx, app), errUpdateApplicationFinalizer) - } - } } else { if meta.FinalizerExists(app, legacyResourceTrackerFinalizer) { // TODO(roywang) legacyResourceTrackerFinalizer will be deprecated in the future @@ -274,19 +265,7 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat meta.RemoveFinalizer(app, legacyResourceTrackerFinalizer) return true, errors.Wrap(r.Client.Update(ctx, app), errUpdateApplicationFinalizer) } - if meta.FinalizerExists(app, resourceTrackerFinalizer) { - if app.Status.LatestRevision != nil && len(app.Status.LatestRevision.Name) != 0 { - latestTracker := &v1beta1.ResourceTracker{} - latestTracker.SetName(dispatch.ConstructResourceTrackerName(app.Status.LatestRevision.Name, app.Namespace)) - if err := r.Client.Delete(ctx, latestTracker); err != nil && !kerrors.IsNotFound(err) { - klog.ErrorS(err, "Failed to delete latest resource tracker", "name", latestTracker.Name) - return true, errors.WithMessage(err, "cannot remove finalizer") - } - } - meta.RemoveFinalizer(app, resourceTrackerFinalizer) - return true, errors.Wrap(r.Client.Update(ctx, app), errUpdateApplicationFinalizer) - } - if meta.FinalizerExists(app, onlyRevisionFinalizer) { + if meta.FinalizerExists(app, resourceTrackerFinalizer) || meta.FinalizerExists(app, legacyOnlyRevisionFinalizer) { listOpts := []client.ListOption{ client.MatchingLabels{ oam.LabelAppName: app.Name, @@ -303,7 +282,10 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat return true, errors.WithMessage(err, "cannot remove finalizer") } } - meta.RemoveFinalizer(app, onlyRevisionFinalizer) + meta.RemoveFinalizer(app, resourceTrackerFinalizer) + // legacyOnlyRevisionFinalizer will be deprecated in the future + // this is for backward compatibility + meta.RemoveFinalizer(app, legacyOnlyRevisionFinalizer) return true, errors.Wrap(r.Client.Update(ctx, app), errUpdateApplicationFinalizer) } } diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go index 80310f5b0..cb9d4f6fa 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go @@ -50,6 +50,9 @@ var _ = Describe("Test application controller finalizer logic", func() { ncd := &v1beta1.ComponentDefinition{} ncdDefJson, _ := yaml.YAMLToJSON([]byte(normalCompDefYaml)) + badCD := &v1beta1.ComponentDefinition{} + badCDJson, _ := yaml.YAMLToJSON([]byte(badCompDefYaml)) + td := &v1beta1.TraitDefinition{} tdDefJson, _ := yaml.YAMLToJSON([]byte(crossNsTdYaml)) @@ -69,6 +72,9 @@ var _ = Describe("Test application controller finalizer logic", func() { Expect(json.Unmarshal(ncdDefJson, ncd)).Should(BeNil()) Expect(k8sClient.Create(ctx, ncd.DeepCopy())).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) + + Expect(json.Unmarshal(badCDJson, badCD)).Should(BeNil()) + Expect(k8sClient.Create(ctx, badCD.DeepCopy())).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) }) AfterEach(func() { @@ -129,6 +135,35 @@ var _ = Describe("Test application controller finalizer logic", func() { Expect(checkApp.Status.ResourceTracker).Should(BeNil()) }) + It("Test error occurs in the middle of dispatching", func() { + appName := "bad-app" + appKey := types.NamespacedName{Namespace: namespace, Name: appName} + app := getApp(appName, namespace, "bad-worker") + Expect(k8sClient.Create(ctx, app)).Should(BeNil()) + + By("Create a bad workload app") + checkApp := &v1beta1.Application{} + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) + Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) + + // because error occurs in the middle of dispatching + // resource tracker for v1 is created but v1 is not recorded in app status + By("Verify latest app revision is not recorded in status") + Expect(checkApp.Status.LatestRevision).Should(BeNil()) + + By("Verify ResourceTracker is created") + rt := &v1beta1.ResourceTracker{} + Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v1"), rt)).Should(Succeed()) + + By("Delete Application") + Expect(k8sClient.Delete(ctx, checkApp)).Should(BeNil()) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) + + By("Verify ResourceTracker is deleted") + rt = &v1beta1.ResourceTracker{} + Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v1"), rt)).Should(util.NotFoundMatcher{}) + }) + It("Test cross namespace workload, then delete the app", func() { appName := "app-2" appKey := types.NamespacedName{Namespace: namespace, Name: appName} @@ -400,4 +435,38 @@ spec: cmd?: [...string] } ` + badCompDefYaml = ` +apiVersion: core.oam.dev/v1beta1 +kind: ComponentDefinition +metadata: + name: bad-worker + namespace: vela-system + annotations: + definition.oam.dev/description: "It will make dispatching failed" +spec: + workload: + definition: + apiVersion: apps/v1 + kind: Deployment + extension: + template: | + output: { + apiVersion: "apps/v1" + kind: "Deployment" + spec: { + replicas: 0 + selector: + matchLabels: + "app.oam.dev/component": context.name + } + } + + parameter: { + // +usage=Which image would you like to use for your service + // +short=i + image: string + + cmd?: [...string] + } +` )