From d8d891f6c9d26413a833638dcc4fc4483703019e Mon Sep 17 00:00:00 2001 From: zzxwill Date: Wed, 30 Dec 2020 00:14:26 +0800 Subject: [PATCH 1/2] Allow trait to work without TraitDefinition Fix the issue of applying trait if its traitdefinition doesn't exits. To fix issue #839 --- ...figuration_without_traitdefinition_test.go | 156 ++++++++++++++++++ .../applicationconfiguration/apply.go | 6 +- .../applicationconfiguration/render.go | 7 +- .../autoscaler/autoscaler_controller.go | 18 +- pkg/oam/util/helper.go | 5 +- 5 files changed, 174 insertions(+), 18 deletions(-) create mode 100644 pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go new file mode 100644 index 000000000..5f3765842 --- /dev/null +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go @@ -0,0 +1,156 @@ +/* +Copyright 2020 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package applicationconfiguration + +import ( + "context" + "strconv" + "time" + + "github.com/ghodss/yaml" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" + "github.com/oam-dev/kubevela/pkg/oam/util" +) + +var _ = Describe("Test Deploying ApplicationConfiguration without TraitDefinition", func() { + const ( + namespace = "definition-test" + appName = "hello" + componentName = "backend" + ) + var ( + ctx = context.Background() + workload v1alpha2.ContainerizedWorkload + component v1alpha2.Component + workloadKey = client.ObjectKey{ + Name: componentName, + Namespace: namespace, + } + appConfig v1alpha2.ApplicationConfiguration + appConfigKey = client.ObjectKey{ + Name: appName, + Namespace: namespace, + } + req = reconcile.Request{NamespacedName: appConfigKey} + ns = corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + } + ) + + BeforeEach(func() {}) + + It("ManualScalerTrait should work successfully even though its TraitDefinition doesn't exist", func() { + var componentStr = ` +apiVersion: core.oam.dev/v1alpha2 +kind: Component +metadata: + name: backend + namespace: definition-test +spec: + workload: + apiVersion: core.oam.dev/v1alpha2 + kind: ContainerizedWorkload + spec: + containers: + - name: nginx + image: nginx:1.9.4 + ports: + - containerPort: 80 + name: nginx + env: + - name: TEST_ENV + value: test + command: [ "/bin/bash", "-c", "--" ] + args: [ "while true; do sleep 30; done;" ] +` + + var appConfigStr = ` +apiVersion: core.oam.dev/v1alpha2 +kind: ApplicationConfiguration +metadata: + name: hello + namespace: definition-test +spec: + components: + - componentName: backend + traits: + - trait: + apiVersion: core.oam.dev/v1alpha2 + kind: ManualScalerTrait + spec: + replicaCount: 3 +` + + By("Create namespace") + Eventually( + func() error { + return k8sClient.Create(ctx, &ns) + }, + time.Second*3, time.Millisecond*300).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) + + By("Create Component") + Expect(yaml.Unmarshal([]byte(componentStr), &component)).Should(BeNil()) + Expect(k8sClient.Create(ctx, &component)).Should(Succeed()) + cmpV1 := &v1alpha2.Component{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: componentName}, cmpV1)).Should(Succeed()) + + By("Create ApplicationConfiguration") + Expect(yaml.Unmarshal([]byte(appConfigStr), &appConfig)).Should(BeNil()) + Expect(k8sClient.Create(ctx, &appConfig)).Should(Succeed()) + + By("Reconcile") + reconcileRetry(reconciler, req) + + By("Check workload created successfully") + Eventually(func() error { + return k8sClient.Get(ctx, workloadKey, &workload) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + + By("Check reconcile again and no error will happen") + reconcileRetry(reconciler, req) + + By("Check appConfig condition should not have error") + Eventually(func() string { + By("Reconcile again and should not have error") + reconcileRetry(reconciler, req) + err := k8sClient.Get(ctx, appConfigKey, &appConfig) + if err != nil { + return err.Error() + } + if len(appConfig.Status.Conditions) != 1 { + return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions)) + } + return string(appConfig.Status.Conditions[0].Reason) + }, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess")) + }) + + AfterEach(func() { + // delete the namespace with all its resources + Expect(k8sClient.Delete(ctx, &ns, client.PropagationPolicy(metav1.DeletePropagationForeground))). + Should(SatisfyAny(BeNil(), &util.NotFoundMatcher{})) + }) + +}) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go index 85e54703c..63323ac8c 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go @@ -77,7 +77,7 @@ func (fn WorkloadApplyFns) Finalize(ctx context.Context, ac *v1alpha2.Applicatio type workloads struct { // use patching-apply for creating/updating Workload patchingClient resource.Applicator - // use updateing-apply for creating/updating Trait + // use updating-apply for creating/updating Trait updatingClient resource.Applicator rawClient client.Client dm discoverymapper.DiscoveryMapper @@ -91,7 +91,7 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, if !wl.HasDep { err := a.patchingClient.Apply(ctx, wl.Workload, ao...) if err != nil { - // TODO(roywang) use errors.As() insteand of type assertion on error + // TODO(roywang) use errors.As() instead of type assertion on error if _, ok := err.(*GenerationUnchanged); !ok { // GenerationUnchanged only aborts applying current workload // but not blocks the whole reconciliation through returning an error @@ -105,7 +105,7 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, } t := trait.Object if err := a.updatingClient.Apply(ctx, &trait.Object, ao...); err != nil { - // TODO(roywang) use errors.As() insteand of type assertion on error + // TODO(roywang) use errors.As() instead of type assertion on error if _, ok := err.(*GenerationUnchanged); !ok { // GenerationUnchanged only aborts applying current trait // but not blocks the whole reconciliation through returning an error diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go index f664eb02c..53aec9627 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go @@ -226,11 +226,10 @@ func (r *components) renderTrait(ctx context.Context, ct v1alpha2.ComponentTrait } traitDef, err := util.FetchTraitDefinition(ctx, r.client, r.dm, t) if err != nil { - if apierrors.IsNotFound(err) { - t.SetNamespace(ac.GetNamespace()) - return t, util.GetDummyTraitDefinition(t), nil + if !apierrors.IsNotFound(err) { + return nil, nil, errors.Wrapf(err, errFmtGetTraitDefinition, t.GetAPIVersion(), t.GetKind(), t.GetName()) } - return nil, nil, errors.Wrapf(err, errFmtGetTraitDefinition, t.GetAPIVersion(), t.GetKind(), t.GetName()) + traitDef = util.GetDummyTraitDefinition(t) } traitName := getTraitName(ac, componentName, &ct, t, traitDef) diff --git a/pkg/controller/standard.oam.dev/v1alpha1/autoscaler/autoscaler_controller.go b/pkg/controller/standard.oam.dev/v1alpha1/autoscaler/autoscaler_controller.go index 9957baf8f..61579a6c1 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/autoscaler/autoscaler_controller.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/autoscaler/autoscaler_controller.go @@ -30,23 +30,21 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" + "github.com/oam-dev/kubevela/pkg/controller/common" "github.com/oam-dev/kubevela/pkg/oam/discoverymapper" "github.com/oam-dev/kubevela/pkg/oam/util" oamutil "github.com/oam-dev/kubevela/pkg/oam/util" - - "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" - "github.com/oam-dev/kubevela/pkg/controller/common" ) // nolint:golint const ( - SpecWarningTargetWorkloadNotSet = "Spec.targetWorkload is not set" - SpecWarningStartAtTimeFormat = "startAt is not in the right format, which should be like `12:01`" - SpecWarningStartAtTimeRequired = "spec.triggers.condition.startAt: Required value" - SpecWarningDurationTimeRequired = "spec.triggers.condition.duration: Required value" - SpecWarningReplicasRequired = "spec.triggers.condition.replicas: Required value" - SpecWarningDurationTimeNotInRightFormat = "spec.triggers.condition.duration: not in the right format" - SpecWarningSumOfStartAndDurationMoreThan24Hour = "the sum of the start hour and the duration hour has to be less than 24 hours." + SpecWarningTargetWorkloadNotSet = "Spec.targetWorkload is not set" + SpecWarningStartAtTimeFormat = "startAt is not in the right format, which should be like `12:01`" + SpecWarningStartAtTimeRequired = "spec.triggers.condition.startAt: Required value" + SpecWarningDurationTimeRequired = "spec.triggers.condition.duration: Required value" + SpecWarningReplicasRequired = "spec.triggers.condition.replicas: Required value" + SpecWarningDurationTimeNotInRightFormat = "spec.triggers.condition.duration: not in the right format" ) // ReconcileWaitResult is the time to wait between reconciliation. diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index ffb3de8a7..d352c849a 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -135,7 +135,10 @@ func GetDummyTraitDefinition(u *unstructured.Unstructured) *v1alpha2.TraitDefini "kind": u.GetKind(), "name": u.GetName(), }}, - Spec: v1alpha2.TraitDefinitionSpec{Reference: v1alpha2.DefinitionReference{Name: Dummy}}, + Spec: v1alpha2.TraitDefinitionSpec{ + Reference: v1alpha2.DefinitionReference{Name: Dummy}, + WorkloadRefPath: "spec.workloadRef", + }, } } From dfdb833abef2ac995522b4210914bcfefb901bf7 Mon Sep 17 00:00:00 2001 From: zzxwill Date: Wed, 30 Dec 2020 14:45:29 +0800 Subject: [PATCH 2/2] Check trait CR is created and update ApplicationConfiguration --- ...figuration_without_traitdefinition_test.go | 80 ++++++++++++++++++- pkg/oam/util/helper.go | 5 +- 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go index 5f3765842..f3e8758c0 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_without_traitdefinition_test.go @@ -18,9 +18,15 @@ package applicationconfiguration import ( "context" + "fmt" "strconv" + "strings" "time" + "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" + + "k8s.io/apimachinery/pkg/runtime" + "github.com/ghodss/yaml" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -101,7 +107,11 @@ spec: apiVersion: core.oam.dev/v1alpha2 kind: ManualScalerTrait spec: - replicaCount: 3 + replicaCount: 2 + workloadRef: + apiVersion: core.oam.dev/v1alpha2 + kind: ContainerizedWorkload + name: backend ` By("Create namespace") @@ -123,6 +133,7 @@ spec: By("Reconcile") reconcileRetry(reconciler, req) + time.Sleep(5) By("Check workload created successfully") Eventually(func() error { @@ -145,6 +156,72 @@ spec: } return string(appConfig.Status.Conditions[0].Reason) }, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess")) + + By("Check trait CR is created") + var scaleName string + scaleList := v1alpha2.ManualScalerTraitList{} + labels := &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app.oam.dev/component": componentName, + }, + } + selector, _ := metav1.LabelSelectorAsSelector(labels) + err := k8sClient.List(ctx, &scaleList, &client.ListOptions{ + Namespace: namespace, + LabelSelector: selector, + }) + Expect(err).Should(BeNil()) + traitNamePrefix := fmt.Sprintf("%s-dummy-", componentName) + var traitExistFlag bool + for _, t := range scaleList.Items { + if strings.HasPrefix(t.Name, traitNamePrefix) { + traitExistFlag = true + scaleName = t.Name + } + } + Expect(traitExistFlag).Should(BeTrue()) + + By("Update ApplicationConfiguration by changing spec of trait") + newTrait := &v1alpha2.ManualScalerTrait{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "core.oam.dev/v1alpha2", + Kind: "ManualScalerTrait", + }, + Spec: v1alpha2.ManualScalerTraitSpec{ + ReplicaCount: 3, + WorkloadReference: v1alpha1.TypedReference{ + APIVersion: "core.oam.dev/v1alpha2", + Kind: "ContainerizedWorkload", + Name: componentName, + }, + }, + } + appConfig.Spec.Components[0].Traits = []v1alpha2.ComponentTrait{{Trait: runtime.RawExtension{Object: newTrait.DeepCopyObject()}}} + Expect(k8sClient.Update(ctx, &appConfig)).Should(BeNil()) + + By("Reconcile") + reconcileRetry(reconciler, req) + + By("Check again that appConfig condition should not have error") + Eventually(func() string { + By("Reconcile again and should not have error") + reconcileRetry(reconciler, req) + err := k8sClient.Get(ctx, appConfigKey, &appConfig) + if err != nil { + return err.Error() + } + if len(appConfig.Status.Conditions) != 1 { + return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions)) + } + return string(appConfig.Status.Conditions[0].Reason) + }, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess")) + + By("Check new trait CR is applied") + scale := v1alpha2.ManualScalerTrait{} + scaleKey := client.ObjectKey{Name: scaleName, Namespace: namespace} + err = k8sClient.Get(ctx, scaleKey, &scale) + Expect(err).Should(BeNil()) + Expect(scale.Spec.ReplicaCount).Should(Equal(int32(3))) }) AfterEach(func() { @@ -152,5 +229,4 @@ spec: Expect(k8sClient.Delete(ctx, &ns, client.PropagationPolicy(metav1.DeletePropagationForeground))). Should(SatisfyAny(BeNil(), &util.NotFoundMatcher{})) }) - }) diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index d352c849a..ffb3de8a7 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -135,10 +135,7 @@ func GetDummyTraitDefinition(u *unstructured.Unstructured) *v1alpha2.TraitDefini "kind": u.GetKind(), "name": u.GetName(), }}, - Spec: v1alpha2.TraitDefinitionSpec{ - Reference: v1alpha2.DefinitionReference{Name: Dummy}, - WorkloadRefPath: "spec.workloadRef", - }, + Spec: v1alpha2.TraitDefinitionSpec{Reference: v1alpha2.DefinitionReference{Name: Dummy}}, } }