From 105ee8365868620533fc40cdb7318016b6727388 Mon Sep 17 00:00:00 2001 From: KubeSphere CI Bot <47586280+ks-ci-bot@users.noreply.github.com> Date: Tue, 15 Aug 2023 18:09:50 +0800 Subject: [PATCH] Fixed the issue of some application resources not being deleted when deleting multiple apps in the cascade (#990) Co-authored-by: stark --- .gitignore | 1 + controllers/argocd/application_controller.go | 130 +++++-- .../argocd/application_controller_test.go | 317 +++++++++++++++++- .../argocd-application-status-controller.go | 2 +- pkg/kapis/gitops/v1alpha1/gitops/handler.go | 3 + 5 files changed, 418 insertions(+), 35 deletions(-) diff --git a/.gitignore b/.gitignore index 93f69539..7d015cf0 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ *.so *.dylib bin +scripts # Test binary, build with `go test -c` *.test diff --git a/controllers/argocd/application_controller.go b/controllers/argocd/application_controller.go index 990532f9..5df88be1 100644 --- a/controllers/argocd/application_controller.go +++ b/controllers/argocd/application_controller.go @@ -95,13 +95,11 @@ func (r *ApplicationReconciler) reconcileArgoApplication(app *v1alpha1.Applicati } err = nil } else { - err = r.Delete(ctx, argoApp) + err = r.DeleteArgoApp(app, argoApp) } if err == nil { - k8sutil.RemoveFinalizer(&app.ObjectMeta, v1alpha1.ApplicationFinalizerName) - k8sutil.RemoveFinalizer(&app.ObjectMeta, v1alpha1.ArgoCDResourcesFinalizer) - err = r.Update(context.TODO(), app) + err = r.RemoveAppFinalizer(app) } return } @@ -153,26 +151,28 @@ func (r *ApplicationReconciler) reconcileArgoApplication(app *v1alpha1.Applicati } else { var newArgoApp *unstructured.Unstructured if newArgoApp, err = createUnstructuredApplication(app); err == nil { - argoApp.Object["spec"] = newArgoApp.Object["spec"] - argoApp.Object["operation"] = newArgoApp.Object["operation"] - - // append annotations and labels - copyArgoAnnotationsAndLabels(newArgoApp, argoApp) - - argoApp.SetFinalizers(newArgoApp.GetFinalizers()) err = retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { - latestArgoApp := createBareArgoCDApplicationObject() - if err = r.Get(context.Background(), types.NamespacedName{ + updateArgoApp := createBareArgoCDApplicationObject() + if err = r.Get(ctx, types.NamespacedName{ Namespace: argoApp.GetNamespace(), Name: argoApp.GetName(), - }, latestArgoApp); err != nil { + }, updateArgoApp); err != nil { return } - argoApp.SetResourceVersion(latestArgoApp.GetResourceVersion()) - err = r.Update(ctx, argoApp) + updateArgoApp.Object["spec"] = newArgoApp.Object["spec"] + updateArgoApp.Object["operation"] = newArgoApp.Object["operation"] + + // append annotations and labels + copyArgoAnnotationsAndLabels(newArgoApp, updateArgoApp) + updateArgoApp.SetFinalizers(newArgoApp.GetFinalizers()) + + err = r.Update(ctx, updateArgoApp) return }) + if err != nil { + return + } } } } @@ -261,18 +261,22 @@ func copyArgoAnnotationsAndLabels(app metav1.Object, argoApp metav1.Object) { } func (r *ApplicationReconciler) addArgoAppNameIntoLabels(namespace, name, argoAppName string) (err error) { - app := &v1alpha1.Application{} ctx := context.Background() - if err = r.Client.Get(ctx, types.NamespacedName{ - Namespace: namespace, - Name: name, - }, app); err == nil { - if app.Labels == nil { - app.Labels = make(map[string]string) + + err = retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { + updateApp := &v1alpha1.Application{} + if err = r.Client.Get(ctx, types.NamespacedName{ + Namespace: namespace, + Name: name, + }, updateApp); err == nil { + if updateApp.Labels == nil { + updateApp.Labels = make(map[string]string) + } + updateApp.Labels[v1alpha1.ArgoCDAppNameLabelKey] = argoAppName + err = r.Update(ctx, updateApp) } - app.Labels[v1alpha1.ArgoCDAppNameLabelKey] = argoAppName - err = r.Update(ctx, app) - } + return + }) return } @@ -290,7 +294,6 @@ func (r *ApplicationReconciler) setArgoProject(app *v1alpha1.Application) (err e }, latestApp); err != nil { return client.IgnoreNotFound(err) } - err = r.Update(ctx, latestApp) // there is a appProject in the same namespace needUpdate := false @@ -299,14 +302,83 @@ func (r *ApplicationReconciler) setArgoProject(app *v1alpha1.Application) (err e needUpdate = true } - if k8sutil.AddFinalizer(&latestApp.ObjectMeta, v1alpha1.ApplicationFinalizerName) || needUpdate { - if err = r.Update(context.TODO(), latestApp); err != nil { + if k8sutil.AddFinalizer(&latestApp.ObjectMeta, v1alpha1.ApplicationFinalizerName) { + needUpdate = true + } + + if needUpdate { + if err = retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { + updateApp := &v1alpha1.Application{} + if err = r.Get(ctx, types.NamespacedName{ + Namespace: app.Namespace, + Name: app.Name, + }, updateApp); err != nil { + return + } + updateApp.Spec.ArgoApp.Spec.Project = latestApp.Spec.ArgoApp.Spec.Project + updateApp.ObjectMeta.Finalizers = latestApp.ObjectMeta.Finalizers + err = r.Update(ctx, updateApp) + return + }); err != nil { return } } return } +// RemoveAppFinalizer the app will be deleted after remove finalizer +func (r *ApplicationReconciler) RemoveAppFinalizer(app *v1alpha1.Application) (err error) { + ctx := context.Background() + + err = retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { + if err = r.Get(ctx, types.NamespacedName{ + Namespace: app.Namespace, + Name: app.Name, + }, app); err != nil { + return + } + k8sutil.RemoveFinalizer(&app.ObjectMeta, v1alpha1.ApplicationFinalizerName) + k8sutil.RemoveFinalizer(&app.ObjectMeta, v1alpha1.ArgoCDResourcesFinalizer) + err = r.Update(ctx, app) + return + }) + return +} + +// DeleteArgoApp will delete argo app, support cascade delete +func (r *ApplicationReconciler) DeleteArgoApp(app *v1alpha1.Application, argoApp *unstructured.Unstructured) (err error) { + ctx := context.Background() + + // support cascade delete + finalizers := app.GetFinalizers() + targetFinalizers := make([]string, 0) + for i := range finalizers { + finalizer := finalizers[i] + if strings.HasSuffix(finalizer, "argocd.argoproj.io") { + targetFinalizers = append(targetFinalizers, finalizer) + } + } + if len(targetFinalizers) > 0 { + err = retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { + updateArgoApp := createBareArgoCDApplicationObject() + if err = r.Get(ctx, types.NamespacedName{ + Namespace: argoApp.GetNamespace(), + Name: argoApp.GetName(), + }, updateArgoApp); err != nil { + return + } + updateArgoApp.SetFinalizers(targetFinalizers) + err = r.Update(ctx, updateArgoApp) + return + }) + if err != nil { + return + } + } + + return r.Delete(ctx, argoApp) +} + // GetName returns the name of this reconciler func (r *ApplicationReconciler) GetName() string { return "ApplicationController" diff --git a/controllers/argocd/application_controller_test.go b/controllers/argocd/application_controller_test.go index 664d8417..036e0e97 100644 --- a/controllers/argocd/application_controller_test.go +++ b/controllers/argocd/application_controller_test.go @@ -210,7 +210,7 @@ func TestApplicationReconciler_reconcileArgoApplication(t *testing.T) { }{{ name: "without Argo Application", fields: fields{ - Client: fake.NewFakeClientWithScheme(schema, app.DeepCopy()), + Client: fake.NewClientBuilder().WithScheme(schema).WithObjects(app.DeepCopy()).Build(), }, args: args{ app: app.DeepCopy(), @@ -233,7 +233,7 @@ func TestApplicationReconciler_reconcileArgoApplication(t *testing.T) { }, { name: "with Argo Application", fields: fields{ - Client: fake.NewFakeClientWithScheme(schema, app.DeepCopy()), + Client: fake.NewClientBuilder().WithScheme(schema).WithObjects(app.DeepCopy()).Build(), }, args: args{ app: app.DeepCopy(), @@ -258,7 +258,7 @@ func TestApplicationReconciler_reconcileArgoApplication(t *testing.T) { }, { name: "update the existing argo application which not have labels", fields: fields{ - Client: fake.NewFakeClientWithScheme(schema, app.DeepCopy(), argoAppList.DeepCopy()), + Client: fake.NewClientBuilder().WithScheme(schema).WithObjects(app.DeepCopy()).WithLists(argoAppList.DeepCopy()).Build(), }, args: args{ app: app.DeepCopy(), @@ -269,7 +269,7 @@ func TestApplicationReconciler_reconcileArgoApplication(t *testing.T) { }, { name: "update the existing argo application which has labels", fields: fields{ - Client: fake.NewFakeClientWithScheme(schema, appWithLabel.DeepCopy(), argoAppWithLabel.DeepCopy()), + Client: fake.NewClientBuilder().WithScheme(schema).WithObjects(appWithLabel.DeepCopy()).WithObjects(argoAppWithLabel.DeepCopy()).Build(), }, args: args{ app: appWithLabel.DeepCopy(), @@ -665,7 +665,7 @@ func TestApplicationReconciler_SetupWithManager(t *testing.T) { name: "normal", args: args{ mgr: &core.FakeManager{ - Client: fake.NewFakeClientWithScheme(schema), + Client: fake.NewClientBuilder().WithScheme(schema).Build(), Scheme: schema, }, }, @@ -682,3 +682,310 @@ func TestApplicationReconciler_SetupWithManager(t *testing.T) { }) } } + +func TestApplicationReconciler_deleteArgoApp(t *testing.T) { + schema, err := v1alpha1.SchemeBuilder.Register().Build() + assert.Nil(t, err) + + ctx := context.Background() + baseArgoApp := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "argoproj.io/v1alpha1", + "kind": "Application", + "metadata": map[string]interface{}{ + "name": "test-app", + "namespace": "argocd", + }, + }, + } + + tests := []struct { + name string + verify func(*testing.T) + }{ + { + name: "cascade", + verify: func(t *testing.T) { + argoApp := baseArgoApp.DeepCopy() + app := &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{"finalizer-1", "resources-finalizer.argocd.argoproj.io", "finalizer-2"}, + }, + } + expectedFinalizers := []string{"resources-finalizer.argocd.argoproj.io"} + r := &ApplicationReconciler{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(baseArgoApp.DeepCopy()).Build(), + log: logr.Logger{}, + } + err = r.DeleteArgoApp(app, argoApp) + assert.NoError(t, err) + + err = r.Get(ctx, types.NamespacedName{ + Namespace: argoApp.GetNamespace(), + Name: argoApp.GetName(), + }, argoApp) + + assert.NoError(t, err) + assert.NotNil(t, argoApp.GetDeletionTimestamp()) + assert.Equal(t, argoApp.GetFinalizers(), expectedFinalizers) + }, + }, + { + name: "cascade with other finalizer", + verify: func(t *testing.T) { + argoApp := baseArgoApp.DeepCopy() + argoApp.SetFinalizers([]string{"test-finalizer.argocd.argoproj.io"}) + app := &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{"finalizer-1", "resources-finalizer.argocd.argoproj.io", "finalizer-2"}, + }, + } + expectedFinalizers := []string{"resources-finalizer.argocd.argoproj.io"} + r := &ApplicationReconciler{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(baseArgoApp.DeepCopy()).Build(), + log: logr.Logger{}, + } + err = r.DeleteArgoApp(app, argoApp) + assert.NoError(t, err) + + err = r.Get(ctx, types.NamespacedName{ + Namespace: argoApp.GetNamespace(), + Name: argoApp.GetName(), + }, argoApp) + + assert.NoError(t, err) + assert.NotNil(t, argoApp.GetDeletionTimestamp()) + assert.Equal(t, argoApp.GetFinalizers(), expectedFinalizers) + }, + }, + { + name: "normal", + verify: func(t *testing.T) { + argoApp := baseArgoApp.DeepCopy() + app := &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{"finalizer-1", "finalizer-2"}, + }, + } + r := &ApplicationReconciler{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(baseArgoApp.DeepCopy()).Build(), + log: logr.Logger{}, + } + err = r.DeleteArgoApp(app, argoApp) + assert.NoError(t, err) + + err = r.Get(ctx, types.NamespacedName{ + Namespace: argoApp.GetNamespace(), + Name: argoApp.GetName(), + }, argoApp) + assert.Error(t, err) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.verify(t) + }) + } +} + +func TestApplicationReconciler_addArgoAppNameIntoLabels(t *testing.T) { + schema, err := v1alpha1.SchemeBuilder.Register().Build() + assert.NoError(t, err) + ctx := context.Background() + + baseApp := &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "argocd", + Name: "test-app", + }, + } + + labelApp := baseApp.DeepCopy() + labelApp.SetLabels(map[string]string{ + "test-label": "test", + }) + + sameLabelApp := baseApp.DeepCopy() + sameLabelApp.SetLabels(map[string]string{ + v1alpha1.ArgoCDAppNameLabelKey: "test", + }) + + type fields struct { + Client client.Client + log logr.Logger + recorder record.EventRecorder + } + type args struct { + namespace string + name string + argoAppName string + expectedLabels map[string]string + } + tests := []struct { + name string + fields fields + args args + wantErr bool + }{ + { + name: "empty label", + fields: fields{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(baseApp.DeepCopy()).Build(), + log: logr.Logger{}, + }, + args: args{ + namespace: "argocd", + name: "test-app", + argoAppName: "test-argo-app", + expectedLabels: map[string]string{ + v1alpha1.ArgoCDAppNameLabelKey: "test-argo-app", + }, + }, + wantErr: false, + }, + { + name: "with label", + fields: fields{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(labelApp.DeepCopy()).Build(), + log: logr.Logger{}, + }, + args: args{ + namespace: "argocd", + name: "test-app", + argoAppName: "test-argo-app", + expectedLabels: map[string]string{ + v1alpha1.ArgoCDAppNameLabelKey: "test-argo-app", + "test-label": "test", + }, + }, + wantErr: false, + }, + { + name: "same label", + fields: fields{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(sameLabelApp.DeepCopy()).Build(), + log: logr.Logger{}, + }, + args: args{ + namespace: "argocd", + name: "test-app", + argoAppName: "test-argo-app", + expectedLabels: map[string]string{ + v1alpha1.ArgoCDAppNameLabelKey: "test-argo-app", + }, + }, + wantErr: false, + }, + { + name: "not exist app", + fields: fields{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(sameLabelApp.DeepCopy()).Build(), + log: logr.Logger{}, + }, + args: args{ + namespace: "not-exist", + name: "not-exist", + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &ApplicationReconciler{ + Client: tt.fields.Client, + log: tt.fields.log, + recorder: tt.fields.recorder, + } + if err := r.addArgoAppNameIntoLabels(tt.args.namespace, tt.args.name, tt.args.argoAppName); (err != nil) != tt.wantErr { + t.Errorf("addArgoAppNameIntoLabels() error = %v, wantErr %v", err, tt.wantErr) + } + if !tt.wantErr { + app := &v1alpha1.Application{} + err = r.Client.Get(ctx, types.NamespacedName{ + Namespace: tt.args.namespace, + Name: tt.args.name, + }, app) + assert.NoError(t, err) + assert.Equal(t, app.Labels, tt.args.expectedLabels) + } + }) + } +} + +func TestApplicationReconciler_RemoveAppFinalizer(t *testing.T) { + schema, err := v1alpha1.SchemeBuilder.Register().Build() + assert.NoError(t, err) + ctx := context.Background() + + baseApp := &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "argocd", + Name: "test-app", + }, + } + + noFinalizersApp := baseApp.DeepCopy() + + finalizersApp := baseApp.DeepCopy() + finalizersApp.SetFinalizers([]string{v1alpha1.ApplicationFinalizerName, v1alpha1.ArgoCDResourcesFinalizer}) + + type fields struct { + Client client.Client + log logr.Logger + recorder record.EventRecorder + } + type args struct { + app *v1alpha1.Application + } + tests := []struct { + name string + fields fields + args args + wantErr bool + }{ + { + name: "empty finalizer", + fields: fields{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(noFinalizersApp).Build(), + log: logr.Logger{}, + }, + args: args{ + app: noFinalizersApp, + }, + wantErr: false, + }, + { + name: "with finalizer", + fields: fields{ + Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(finalizersApp).Build(), + log: logr.Logger{}, + }, + args: args{ + app: finalizersApp, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &ApplicationReconciler{ + Client: tt.fields.Client, + log: tt.fields.log, + recorder: tt.fields.recorder, + } + if err := r.RemoveAppFinalizer(tt.args.app); (err != nil) != tt.wantErr { + t.Errorf("RemoveAppFinalizer() error = %v, wantErr %v", err, tt.wantErr) + } + if !tt.wantErr { + app := &v1alpha1.Application{} + err = r.Client.Get(ctx, types.NamespacedName{ + Namespace: tt.args.app.Namespace, + Name: tt.args.app.Name, + }, app) + assert.NoError(t, err) + assert.Empty(t, app.Finalizers) + } + }) + } +} diff --git a/controllers/argocd/argocd-application-status-controller.go b/controllers/argocd/argocd-application-status-controller.go index 44ac35dc..98461cc3 100644 --- a/controllers/argocd/argocd-application-status-controller.go +++ b/controllers/argocd/argocd-application-status-controller.go @@ -66,7 +66,7 @@ func (r *ApplicationStatusReconciler) Reconcile(ctx context.Context, req ctrl.Re Namespace: appNs, Name: appName, }, app); err != nil { - r.log.Error(err, "cannot find application with namespace: %s, name: %s", appNs, appName) + r.log.Error(err, "cannot find application, maybe deleted", "namespace", appNs, "name", appName) err = nil return } diff --git a/pkg/kapis/gitops/v1alpha1/gitops/handler.go b/pkg/kapis/gitops/v1alpha1/gitops/handler.go index 86f7f642..8c72382b 100644 --- a/pkg/kapis/gitops/v1alpha1/gitops/handler.go +++ b/pkg/kapis/gitops/v1alpha1/gitops/handler.go @@ -19,6 +19,7 @@ import ( "context" "github.com/emicklei/go-restful" "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" "kubesphere.io/devops/pkg/api/gitops/v1alpha1" "kubesphere.io/devops/pkg/apiserver/query" "kubesphere.io/devops/pkg/kapis/common" @@ -100,6 +101,8 @@ func (h *Handler) DelApplication(req *restful.Request, res *restful.Response) { } } } + } else { + klog.Errorf("Application %s in namespace %s is not type argocd", name, namespace) } err = h.Delete(ctx, application) }