From 4729c8af80bf7078f6268353ae9281652bc2de14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Mon, 29 Mar 2021 23:10:59 +0800 Subject: [PATCH 1/7] use Application as the owner if it exists --- .../v1alpha2/applicationconfiguration/render.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go index 01abcd5fe..6dbe1f880 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go @@ -202,7 +202,8 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati util.PassLabelAndAnnotation(ac, w) // don't pass the following annotation as those are for appConfig only util.RemoveAnnotations(w, []string{oam.AnnotationAppRollout, oam.AnnotationRollingComponent, oam.AnnotationInplaceUpgrade}) - ref := metav1.NewControllerRef(ac, v1alpha2.ApplicationConfigurationGroupVersionKind) + ref := getOwnerFromAC(ac) + // Don't override if the resources already has namespace, it was set by user or the application controller which is by design. if len(w.GetNamespace()) == 0 { w.SetNamespace(ac.GetNamespace()) @@ -817,6 +818,18 @@ func isControlledByApp(ac *v1alpha2.ApplicationConfiguration) bool { return false } +// getOwnerFromAC will check and get the real owner, if the owner is Application, it will use Application as owner +// or it will make the AC as the owner +func getOwnerFromAC(ac *v1alpha2.ApplicationConfiguration) *metav1.OwnerReference { + for _, owner := range ac.GetOwnerReferences() { + if owner.APIVersion == v1beta1.SchemeGroupVersion.String() && owner.Kind == v1beta1.ApplicationKind && + owner.Controller != nil && *owner.Controller { + return &owner + } + } + return metav1.NewControllerRef(ac, v1alpha2.ApplicationConfigurationGroupVersionKind) +} + func matchValue(conds []v1alpha2.ConditionRequirement, val string, paved, ac *fieldpath.Paved) (bool, string) { // If no condition is specified, it is by default to check value not empty. if len(conds) == 0 { From ffe08904f099a5910f1b60a9dd586e0a0d9a7a95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Mon, 29 Mar 2021 23:31:52 +0800 Subject: [PATCH 2/7] add a todo for fix by the ownerRef change --- pkg/oam/util/helper.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index e8d6183e3..421e8de11 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -136,6 +136,7 @@ func LocateParentAppConfig(ctx context.Context, client client.Client, oamObject var eventObj = &v1alpha2.ApplicationConfiguration{} // locate the appConf name from the owner list for _, o := range oamObject.GetOwnerReferences() { + // TODO(wonderflow): this function maybe not work in the case AC was created by Application and the Application is the ownerRef here. if o.Kind == v1alpha2.ApplicationConfigurationKind { acName = o.Name break From 0d8f656cdb074738124bdb8debd94cea39f1bab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Mon, 29 Mar 2021 23:45:29 +0800 Subject: [PATCH 3/7] fix some tests --- .../applicationcontext_controller.go | 7 ++++ pkg/oam/util/helper.go | 42 ++++++++++++------- test/e2e-test/app_resourcetracker_test.go | 13 +++--- test/e2e-test/rollout_plan_test.go | 2 +- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go b/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go index d36ccf55b..8b660b184 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go @@ -102,6 +102,13 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // the name of the appConfig has to be the same as the appContext appConfig.Name = appContext.Name appConfig.UID = appContext.UID + for _, owner := range appContext.GetOwnerReferences() { + // if the appContext is created by Application, set the AppConfig UID to align with the application + if owner.Kind == v1beta1.ApplicationKind { + appConfig.UID = owner.UID + break + } + } appConfig.SetLabels(appContext.GetLabels()) appConfig.SetAnnotations(appContext.GetAnnotations()) // makes sure that the appConfig's owner is the same as the appContext diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index 421e8de11..f1f3b406a 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -27,6 +27,8 @@ import ( "strings" "time" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + cpv1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/davecgh/go-spew/spew" "github.com/go-logr/logr" @@ -132,25 +134,37 @@ type ConditionedObject interface { // LocateParentAppConfig locate the parent application configuration object func LocateParentAppConfig(ctx context.Context, client client.Client, oamObject oam.Object) (oam.Object, error) { - var acName string - var eventObj = &v1alpha2.ApplicationConfiguration{} + // locate the appConf name from the owner list for _, o := range oamObject.GetOwnerReferences() { - // TODO(wonderflow): this function maybe not work in the case AC was created by Application and the Application is the ownerRef here. if o.Kind == v1alpha2.ApplicationConfigurationKind { - acName = o.Name - break + var eventObj = &v1alpha2.ApplicationConfiguration{} + acName := o.Name + if len(acName) > 0 { + nn := types.NamespacedName{ + Name: acName, + Namespace: oamObject.GetNamespace(), + } + if err := client.Get(ctx, nn, eventObj); err != nil { + return nil, err + } + return eventObj, nil + } } - } - if len(acName) > 0 { - nn := types.NamespacedName{ - Name: acName, - Namespace: oamObject.GetNamespace(), + if o.Kind == v1beta1.ApplicationKind { + var eventObj = &v1beta1.Application{} + appName := o.Name + if len(appName) > 0 { + nn := types.NamespacedName{ + Name: appName, + Namespace: oamObject.GetNamespace(), + } + if err := client.Get(ctx, nn, eventObj); err != nil { + return nil, err + } + return eventObj, nil + } } - if err := client.Get(ctx, nn, eventObj); err != nil { - return nil, err - } - return eventObj, nil } return nil, errors.Errorf(ErrLocateAppConfig) } diff --git a/test/e2e-test/app_resourcetracker_test.go b/test/e2e-test/app_resourcetracker_test.go index ef7a4b73c..75677aa27 100644 --- a/test/e2e-test/app_resourcetracker_test.go +++ b/test/e2e-test/app_resourcetracker_test.go @@ -25,11 +25,6 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "github.com/oam-dev/kubevela/apis/core.oam.dev/common" - "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" - "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" - "github.com/oam-dev/kubevela/pkg/oam" - "github.com/oam-dev/kubevela/pkg/oam/util" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -38,6 +33,12 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/yaml" + + "github.com/oam-dev/kubevela/apis/core.oam.dev/common" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + "github.com/oam-dev/kubevela/pkg/oam" + "github.com/oam-dev/kubevela/pkg/oam/util" ) var _ = Describe("Test application cross namespace resource", func() { @@ -240,7 +241,7 @@ var _ = Describe("Test application cross namespace resource", func() { return fmt.Errorf("error workload number %v", err) } workload := depolys.Items[0] - if len(workload.OwnerReferences) != 1 || workload.OwnerReferences[0].Kind != v1alpha2.ApplicationConfigurationKind { + if len(workload.OwnerReferences) != 1 || workload.OwnerReferences[0].Kind != v1beta1.ApplicationKind { return fmt.Errorf("workload owneRefernece err") } err = k8sClient.Get(ctx, generateResourceTrackerKey(app.Namespace, app.Name), resourceTracker) diff --git a/test/e2e-test/rollout_plan_test.go b/test/e2e-test/rollout_plan_test.go index aa72fb8a8..db8bfc59a 100644 --- a/test/e2e-test/rollout_plan_test.go +++ b/test/e2e-test/rollout_plan_test.go @@ -182,7 +182,7 @@ var _ = Describe("Cloneset based rollout tests", func() { } return "" }, - time.Second*30, time.Millisecond*500).Should(BeEquivalentTo(v1alpha2.ApplicationConfigurationKind)) + time.Second*30, time.Millisecond*500).Should(BeEquivalentTo(v1beta1.ApplicationKind)) Expect(clonesetOwner.Name).Should(BeEquivalentTo(targetAppName)) Expect(kc.Status.UpdatedReplicas).Should(BeEquivalentTo(*kc.Spec.Replicas)) Expect(kc.Status.UpdatedReadyReplicas).Should(BeEquivalentTo(*kc.Spec.Replicas)) From 4918b5cee72ddf9cf9517d48d0c2c33c58aabf5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Mon, 29 Mar 2021 23:48:51 +0800 Subject: [PATCH 4/7] fix test --- test/e2e-test/app_resourcetracker_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e-test/app_resourcetracker_test.go b/test/e2e-test/app_resourcetracker_test.go index 75677aa27..dfe22f758 100644 --- a/test/e2e-test/app_resourcetracker_test.go +++ b/test/e2e-test/app_resourcetracker_test.go @@ -486,7 +486,7 @@ var _ = Describe("Test application cross namespace resource", func() { return fmt.Errorf("failed generate same namespace workload") } sameDeplpoy := same.Items[0] - if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1alpha2.ApplicationConfigurationKind { + if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1beta1.ApplicationKind { return fmt.Errorf("same ns deploy have error ownerReference") } err = k8sClient.List(ctx, cross, crossOpts...) @@ -539,7 +539,7 @@ var _ = Describe("Test application cross namespace resource", func() { return fmt.Errorf("failed generate same namespace workload") } sameDeplpoy := same.Items[0] - if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1alpha2.ApplicationConfigurationKind { + if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1beta1.ApplicationKind { return fmt.Errorf("same ns deploy have error ownerReference") } err = k8sClient.List(ctx, cross, crossOpts...) From 72f533e5eb271d3fd4bc731da130a25d086580dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Tue, 30 Mar 2021 00:21:17 +0800 Subject: [PATCH 5/7] use AppContext as the ownerRef --- .../v1alpha2/applicationconfiguration/render.go | 4 ++-- .../applicationcontext/applicationcontext_controller.go | 7 ------- pkg/oam/util/helper.go | 2 +- test/e2e-test/app_resourcetracker_test.go | 6 +++--- test/e2e-test/rollout_plan_test.go | 2 +- 5 files changed, 7 insertions(+), 14 deletions(-) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go index 6dbe1f880..687eb7277 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go @@ -818,13 +818,13 @@ func isControlledByApp(ac *v1alpha2.ApplicationConfiguration) bool { return false } -// getOwnerFromAC will check and get the real owner, if the owner is Application, it will use Application as owner +// getOwnerFromAC will check and get the real owner, if the owner is Application, it will use ApplicationContext as owner // or it will make the AC as the owner func getOwnerFromAC(ac *v1alpha2.ApplicationConfiguration) *metav1.OwnerReference { for _, owner := range ac.GetOwnerReferences() { if owner.APIVersion == v1beta1.SchemeGroupVersion.String() && owner.Kind == v1beta1.ApplicationKind && owner.Controller != nil && *owner.Controller { - return &owner + return metav1.NewControllerRef(ac, v1alpha2.ApplicationContextGroupVersionKind) } } return metav1.NewControllerRef(ac, v1alpha2.ApplicationConfigurationGroupVersionKind) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go b/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go index 8b660b184..d36ccf55b 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go @@ -102,13 +102,6 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // the name of the appConfig has to be the same as the appContext appConfig.Name = appContext.Name appConfig.UID = appContext.UID - for _, owner := range appContext.GetOwnerReferences() { - // if the appContext is created by Application, set the AppConfig UID to align with the application - if owner.Kind == v1beta1.ApplicationKind { - appConfig.UID = owner.UID - break - } - } appConfig.SetLabels(appContext.GetLabels()) appConfig.SetAnnotations(appContext.GetAnnotations()) // makes sure that the appConfig's owner is the same as the appContext diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index f1f3b406a..aa51493c4 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -152,7 +152,7 @@ func LocateParentAppConfig(ctx context.Context, client client.Client, oamObject } } if o.Kind == v1beta1.ApplicationKind { - var eventObj = &v1beta1.Application{} + var eventObj = &v1alpha2.ApplicationContext{} appName := o.Name if len(appName) > 0 { nn := types.NamespacedName{ diff --git a/test/e2e-test/app_resourcetracker_test.go b/test/e2e-test/app_resourcetracker_test.go index dfe22f758..2c40d53d4 100644 --- a/test/e2e-test/app_resourcetracker_test.go +++ b/test/e2e-test/app_resourcetracker_test.go @@ -241,7 +241,7 @@ var _ = Describe("Test application cross namespace resource", func() { return fmt.Errorf("error workload number %v", err) } workload := depolys.Items[0] - if len(workload.OwnerReferences) != 1 || workload.OwnerReferences[0].Kind != v1beta1.ApplicationKind { + if len(workload.OwnerReferences) != 1 || workload.OwnerReferences[0].Kind != v1alpha2.ApplicationContextKind { return fmt.Errorf("workload owneRefernece err") } err = k8sClient.Get(ctx, generateResourceTrackerKey(app.Namespace, app.Name), resourceTracker) @@ -486,7 +486,7 @@ var _ = Describe("Test application cross namespace resource", func() { return fmt.Errorf("failed generate same namespace workload") } sameDeplpoy := same.Items[0] - if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1beta1.ApplicationKind { + if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1alpha2.ApplicationContextKind { return fmt.Errorf("same ns deploy have error ownerReference") } err = k8sClient.List(ctx, cross, crossOpts...) @@ -539,7 +539,7 @@ var _ = Describe("Test application cross namespace resource", func() { return fmt.Errorf("failed generate same namespace workload") } sameDeplpoy := same.Items[0] - if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1beta1.ApplicationKind { + if len(sameDeplpoy.OwnerReferences) != 1 || sameDeplpoy.OwnerReferences[0].Kind != v1alpha2.ApplicationContextKind { return fmt.Errorf("same ns deploy have error ownerReference") } err = k8sClient.List(ctx, cross, crossOpts...) diff --git a/test/e2e-test/rollout_plan_test.go b/test/e2e-test/rollout_plan_test.go index db8bfc59a..2bbbf9a74 100644 --- a/test/e2e-test/rollout_plan_test.go +++ b/test/e2e-test/rollout_plan_test.go @@ -182,7 +182,7 @@ var _ = Describe("Cloneset based rollout tests", func() { } return "" }, - time.Second*30, time.Millisecond*500).Should(BeEquivalentTo(v1beta1.ApplicationKind)) + time.Second*30, time.Millisecond*500).Should(BeEquivalentTo(v1alpha2.ApplicationContextKind)) Expect(clonesetOwner.Name).Should(BeEquivalentTo(targetAppName)) Expect(kc.Status.UpdatedReplicas).Should(BeEquivalentTo(*kc.Spec.Replicas)) Expect(kc.Status.UpdatedReadyReplicas).Should(BeEquivalentTo(*kc.Spec.Replicas)) From 78ed5cade0d5ffcb812f08fefa3dd3fb724c4d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Tue, 30 Mar 2021 00:24:10 +0800 Subject: [PATCH 6/7] minor fix --- pkg/oam/util/helper.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index aa51493c4..537ae7d49 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -27,8 +27,6 @@ import ( "strings" "time" - "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" - cpv1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/davecgh/go-spew/spew" "github.com/go-logr/logr" @@ -151,7 +149,7 @@ func LocateParentAppConfig(ctx context.Context, client client.Client, oamObject return eventObj, nil } } - if o.Kind == v1beta1.ApplicationKind { + if o.Kind == v1alpha2.ApplicationContextKind { var eventObj = &v1alpha2.ApplicationContext{} appName := o.Name if len(appName) > 0 { From 74a95133b26eaf477122e61565f7cec9fde10fdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Tue, 30 Mar 2021 00:39:18 +0800 Subject: [PATCH 7/7] fix log --- test/e2e-test/application_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e-test/application_test.go b/test/e2e-test/application_test.go index 1017a989d..98f7e1d54 100644 --- a/test/e2e-test/application_test.go +++ b/test/e2e-test/application_test.go @@ -106,7 +106,7 @@ var _ = Describe("Application Normal tests", func() { verifyWorkloadRunningExpected := func(workloadName string, replicas int32, image string) { var workload v1.Deployment - By("Verify AppConfig is inactive") + By("Verify Workload running as expected") Eventually( func() error { if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespaceName, Name: workloadName}, &workload); err != nil {