From f5b06f855a97a2b5aa36b72cdc60b4fe7b8d8527 Mon Sep 17 00:00:00 2001 From: wyike Date: Thu, 4 Nov 2021 16:40:02 +0800 Subject: [PATCH] Fix: op.delete bugs (#2622) * Fix: op.delete some bugs * Fix: app status update error Fix: make reviewable --- .../application/application_controller.go | 2 +- .../v1alpha2/application/apply.go | 29 ++++ .../v1alpha2/application/apply_test.go | 124 ++++++++++++++++++ .../v1alpha2/application/generator.go | 2 +- pkg/stdlib/pkgs/kube.cue | 1 + pkg/workflow/providers/kube/handle.go | 19 ++- pkg/workflow/providers/kube/handle_test.go | 6 + 7 files changed, 174 insertions(+), 9 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 fe04c2808..e8d54ed2e 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go @@ -169,6 +169,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.Recorder.Event(app, event.Normal(velatypes.ReasonRendered, velatypes.MessageRendered)) if !appWillRollout(app) { + handler.addAppliedResource(app.Status.AppliedResources...) steps, err := handler.GenerateApplicationSteps(ctx, app, appParser, appFile, handler.currentAppRev) if err != nil { klog.Error(err, "[handle workflow]") @@ -185,7 +186,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } handler.addServiceStatus(false, app.Status.Services...) - handler.addAppliedResource(app.Status.AppliedResources...) app.Status.AppliedResources = handler.appliedResources app.Status.Services = handler.services switch workflowState { diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/apply.go b/pkg/controller/core.oam.dev/v1alpha2/application/apply.go index 1d1f7ad49..23c669e30 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/apply.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/apply.go @@ -81,6 +81,25 @@ func (h *AppHandler) Dispatch(ctx context.Context, cluster string, owner common. return err } +// Delete delete manifests from k8s. +func (h *AppHandler) Delete(ctx context.Context, cluster string, owner common.ResourceCreatorRole, manifest *unstructured.Unstructured) error { + if err := h.r.Delete(ctx, manifest); err != nil { + return err + } + ref := common.ClusterObjectReference{ + Cluster: cluster, + Creator: owner, + ObjectReference: corev1.ObjectReference{ + Name: manifest.GetName(), + Namespace: manifest.GetNamespace(), + Kind: manifest.GetKind(), + APIVersion: manifest.GetAPIVersion(), + }, + } + h.deleteAppliedResource(ref) + return nil +} + // addAppliedResource recorde applied resource. // reconcile run at single threaded. So there is no need to consider to use locker. func (h *AppHandler) addAppliedResource(refs ...common.ClusterObjectReference) { @@ -98,6 +117,16 @@ func (h *AppHandler) addAppliedResource(refs ...common.ClusterObjectReference) { } } +func (h *AppHandler) deleteAppliedResource(ref common.ClusterObjectReference) { + resouces := []common.ClusterObjectReference{} + for _, current := range h.appliedResources { + if !isSameObjReference(current, ref) { + resouces = append(resouces, current) + } + } + h.appliedResources = resouces +} + func isSameObjReference(ref1, ref2 common.ClusterObjectReference) bool { return ref1.Cluster == ref2.Cluster && ref1.Namespace == ref2.Namespace && diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go index c572c99ed..bc543d61c 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go @@ -20,6 +20,7 @@ import ( "context" "strconv" "strings" + "testing" "time" "github.com/oam-dev/kubevela/pkg/oam/testutil" @@ -28,10 +29,13 @@ import ( terraformapi "github.com/oam-dev/terraform-controller/api/v1beta1" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/yaml" @@ -40,6 +44,7 @@ import ( "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" velatypes "github.com/oam-dev/kubevela/apis/types" "github.com/oam-dev/kubevela/pkg/appfile" + "github.com/oam-dev/kubevela/pkg/oam/util" ) const workloadDefinition = ` @@ -215,3 +220,122 @@ var _ = Describe("Test statusAggregate", func() { Expect(err).Should(BeNil()) }) }) + +var _ = Describe("Test deleter resource", func() { + It("Test delete resource will remove ref from reference", func() { + deployName := "test-del-resource-workload" + namespace := "test-del-resource-namespace" + ctx := context.Background() + Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}})).Should(BeNil()) + deploy := appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: deployName, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: pointer.Int32Ptr(3), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test", + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "app": "test", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "test-image", + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, &deploy)).Should(BeNil()) + u := unstructured.Unstructured{} + u.SetAPIVersion("apps/v1") + u.SetKind("Deployment") + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: deployName, Namespace: namespace}, &u)).Should(BeNil()) + appliedRsc := []common.ClusterObjectReference{ + { + Creator: common.WorkflowResourceCreator, + ObjectReference: corev1.ObjectReference{ + Kind: u.GetKind(), + APIVersion: u.GetAPIVersion(), + Namespace: u.GetNamespace(), + Name: deployName, + }, + }, + { + Creator: common.WorkflowResourceCreator, + ObjectReference: corev1.ObjectReference{ + Kind: "StatefulSet", + APIVersion: "apps/v1", + Namespace: "test-namespace", + Name: "test-sts", + }, + }, + } + h := AppHandler{r: reconciler, appliedResources: appliedRsc} + Expect(h.Delete(ctx, "", common.WorkflowResourceCreator, &u)) + checkDeploy := unstructured.Unstructured{} + checkDeploy.SetAPIVersion("apps/v1") + checkDeploy.SetKind("Deployment") + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: deployName, Namespace: namespace}, &u)).Should(SatisfyAny(util.NotFoundMatcher{})) + Expect(len(h.appliedResources)).Should(BeEquivalentTo(1)) + Expect(h.appliedResources[0].Kind).Should(BeEquivalentTo("StatefulSet")) + Expect(h.appliedResources[0].Name).Should(BeEquivalentTo("test-sts")) + }) +}) + +func TestDeleteAppliedResourceFunc(t *testing.T) { + h := AppHandler{appliedResources: []common.ClusterObjectReference{ + { + ObjectReference: corev1.ObjectReference{ + Name: "wl-1", + Kind: "Deployment", + }, + }, + { + ObjectReference: corev1.ObjectReference{ + Name: "wl-2", + Kind: "Deployment", + }, + }, + { + ObjectReference: corev1.ObjectReference{ + Name: "wl-1", + Kind: "StatefulSet", + }, + }, + { + Cluster: "runtime-cluster", + ObjectReference: corev1.ObjectReference{ + Name: "wl-1", + Kind: "StatefulSet", + }, + }, + }} + deleteResc_1 := common.ClusterObjectReference{ObjectReference: corev1.ObjectReference{Name: "wl-1", Kind: "StatefulSet"}, Cluster: "runtime-cluster"} + deleteResc_2 := common.ClusterObjectReference{ObjectReference: corev1.ObjectReference{Name: "wl-2", Kind: "Deployment"}} + h.deleteAppliedResource(deleteResc_1) + h.deleteAppliedResource(deleteResc_2) + if len(h.appliedResources) != 2 { + t.Errorf("applied length error acctually %d", len(h.appliedResources)) + } + if h.appliedResources[0].Name != "wl-1" || h.appliedResources[0].Kind != "Deployment" { + t.Errorf("resource missmatch") + } + if h.appliedResources[1].Name != "wl-1" || h.appliedResources[1].Kind != "StatefulSet" { + t.Errorf("resource missmatch") + } +} diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/generator.go b/pkg/controller/core.oam.dev/v1alpha2/application/generator.go index f283e65e8..f236f4dc3 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/generator.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/generator.go @@ -48,7 +48,7 @@ func (h *AppHandler) GenerateApplicationSteps(ctx context.Context, af *appfile.Appfile, appRev *v1beta1.ApplicationRevision) ([]wfTypes.TaskRunner, error) { handlerProviders := providers.NewProviders() - kube.Install(handlerProviders, h.r.Client, h.Dispatch) + kube.Install(handlerProviders, h.r.Client, h.Dispatch, h.Delete) oamProvider.Install(handlerProviders, app, h.applyComponentFunc( appParser, appRev, af), h.renderComponentFunc(appParser, appRev, af)) taskDiscover := tasks.NewTaskDiscover(handlerProviders, h.r.pd, h.r.Client, h.r.dm) diff --git a/pkg/stdlib/pkgs/kube.cue b/pkg/stdlib/pkgs/kube.cue index af381fcec..a825ef1e3 100644 --- a/pkg/stdlib/pkgs/kube.cue +++ b/pkg/stdlib/pkgs/kube.cue @@ -41,4 +41,5 @@ namespace: *"default" | string } } + ... } diff --git a/pkg/workflow/providers/kube/handle.go b/pkg/workflow/providers/kube/handle.go index fc178228e..606933ff1 100644 --- a/pkg/workflow/providers/kube/handle.go +++ b/pkg/workflow/providers/kube/handle.go @@ -41,9 +41,13 @@ const ( // Dispatcher is a client for apply resources. type Dispatcher func(ctx context.Context, cluster string, owner common.ResourceCreatorRole, manifests ...*unstructured.Unstructured) error +// Deleter is a client for delete resources. +type Deleter func(ctx context.Context, cluster string, owner common.ResourceCreatorRole, manifest *unstructured.Unstructured) error + type provider struct { - apply Dispatcher - cli client.Client + apply Dispatcher + delete Deleter + cli client.Client } // Apply create or update CR in cluster. @@ -170,18 +174,19 @@ func (h *provider) Delete(ctx wfContext.Context, v *value.Value, act types.Actio if err != nil { return err } - readCtx := multicluster.ContextWithClusterName(context.Background(), cluster) - if err := h.cli.Delete(readCtx, obj); err != nil { + deleteCtx := multicluster.ContextWithClusterName(context.Background(), cluster) + if err := h.delete(deleteCtx, cluster, common.WorkflowResourceCreator, obj); err != nil { return v.FillObject(err.Error(), "err") } return nil } // Install register handlers to provider discover. -func Install(p providers.Providers, cli client.Client, apply Dispatcher) { +func Install(p providers.Providers, cli client.Client, apply Dispatcher, deleter Deleter) { prd := &provider{ - apply: apply, - cli: cli, + apply: apply, + delete: deleter, + cli: cli, } p.Register(ProviderName, map[string]providers.Handler{ "apply": prd.Apply, diff --git a/pkg/workflow/providers/kube/handle_test.go b/pkg/workflow/providers/kube/handle_test.go index 6a62c75f8..a71fb3249 100644 --- a/pkg/workflow/providers/kube/handle_test.go +++ b/pkg/workflow/providers/kube/handle_test.go @@ -282,6 +282,12 @@ cluster: "" apply: func(ctx context.Context, _ string, _ common.ResourceCreatorRole, manifests ...*unstructured.Unstructured) error { return nil }, + delete: func(ctx context.Context, cluster string, owner common.ResourceCreatorRole, manifest *unstructured.Unstructured) error { + if err := k8sClient.Delete(ctx, manifest); err != nil { + return err + } + return nil + }, cli: k8sClient, }