diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go index e1f0df07e..7e1444dea 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go @@ -27,6 +27,7 @@ import ( runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/resource" + kruise "github.com/openkruise/kruise-api/apps/v1alpha1" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -34,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" @@ -60,7 +62,6 @@ const ( errFmtUnsupportedParam = "unsupported parameter %q" errFmtRequiredParam = "required parameter %q not specified" errFmtCompRevision = "cannot get latest revision for component %q while revision is enabled" - errSetValueForField = "can not set value %q for fieldPath %q" ) var ( @@ -68,10 +69,6 @@ var ( ErrDataOutputNotExist = errors.New("DataOutput does not exist") ) -const ( - instanceNamePath = "metadata.name" -) - // A ComponentRenderer renders an ApplicationConfiguration's Components into // workloads and traits. type ComponentRenderer interface { @@ -101,18 +98,8 @@ type components struct { func (r *components) Render(ctx context.Context, ac *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { workloads := make([]*Workload, 0, len(ac.Spec.Components)) dag := newDAG() - // we have special logics for application generated applicationConfiguration - controlledByApp := false - for _, owner := range ac.GetOwnerReferences() { - if owner.APIVersion == v1alpha2.SchemeGroupVersion.String() && owner.Kind == v1alpha2.ApplicationKind && - owner.Controller != nil && *owner.Controller { - controlledByApp = true - break - } - } - for _, acc := range ac.Spec.Components { - w, err := r.renderComponent(ctx, acc, ac, dag, controlledByApp) + w, err := r.renderComponent(ctx, acc, ac, dag) if err != nil { return nil, nil, err } @@ -135,7 +122,10 @@ func (r *components) Render(ctx context.Context, ac *v1alpha2.ApplicationConfigu } func (r *components) renderComponent(ctx context.Context, acc v1alpha2.ApplicationConfigurationComponent, - ac *v1alpha2.ApplicationConfiguration, dag *dag, controlledByApp bool) (*Workload, error) { + ac *v1alpha2.ApplicationConfiguration, dag *dag) (*Workload, error) { + // we have special logics for application generated applicationConfiguration + controlledByApp := isControlledByApp(ac) + if acc.RevisionName != "" { acc.ComponentName = utils.ExtractComponentName(acc.RevisionName) } @@ -192,27 +182,23 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati traits = append(traits, &Trait{Object: *t, Definition: *traitDef}) traitDefs = append(traitDefs, *traitDef) } - // we have a complete different approach on workload name for application generated appConfig if !controlledByApp { + // This is the legacy standalone appConfig approach existingWorkload, err := r.getExistingWorkload(ctx, ac, c, w) if err != nil { return nil, err } - if err := SetWorkloadInstanceName(traitDefs, w, c, existingWorkload); err != nil { + if err := setWorkloadInstanceName(traitDefs, w, c, existingWorkload); err != nil { return nil, err } } else { + // we have a completely different approach on workload name for application generated appConfig revision, err := utils.ExtractRevision(acc.RevisionName) if err != nil { return nil, err } - - pv := fieldpath.Pave(w.UnstructuredContent()) - if err := pv.SetString(instanceNamePath, utils.ConstructRevisionName(acc.ComponentName, - int64(revision))); err != nil { - return nil, errors.Wrapf(err, errSetValueForField, instanceNamePath, c.GetName()) - } + setAppWorkloadInstanceName(acc.ComponentName, w, revision) } // create the ref after the workload name is set @@ -261,8 +247,8 @@ func (r *components) renderTrait(ctx context.Context, ct v1alpha2.ComponentTrait } traitDef = util.GetDummyTraitDefinition(t) } - traitName := getTraitName(ac, componentName, &ct, t, traitDef) + traitName := getTraitName(ac, componentName, &ct, t, traitDef) setTraitProperties(t, traitName, ac.GetNamespace(), ref) addDataOutputsToDAG(dag, ct.DataOutputs, t) @@ -292,14 +278,13 @@ func setTraitProperties(t *unstructured.Unstructured, traitName, namespace strin t.SetNamespace(namespace) } -// SetWorkloadInstanceName will set metadata.name for workload CR according to createRevision flag in traitDefinition -func SetWorkloadInstanceName(traitDefs []v1alpha2.TraitDefinition, w *unstructured.Unstructured, +// setWorkloadInstanceName will set metadata.name for workload CR according to createRevision flag in traitDefinition +func setWorkloadInstanceName(traitDefs []v1alpha2.TraitDefinition, w *unstructured.Unstructured, c *v1alpha2.Component, existingWorkload *unstructured.Unstructured) error { // Don't override the specified name if w.GetName() != "" { return nil } - pv := fieldpath.Pave(w.UnstructuredContent()) // TODO: revisit this logic // the name of the workload should depend on the workload type and if we are rolling or replacing upgrade // i.e Cloneset type of workload just use the component name while deployment type of workload will have revision @@ -313,22 +298,17 @@ func SetWorkloadInstanceName(traitDefs []v1alpha2.TraitDefinition, w *unstructur // if workload exists, check the revision label, we will not change the name if workload exists and no revision changed if existingWorkload != nil && existingWorkload.GetLabels()[oam.LabelAppComponentRevision] == componentLastRevision { // using the existing name - return errors.Wrapf(pv.SetString(instanceNamePath, existingWorkload.GetName()), errSetValueForField, instanceNamePath, c.Status.LatestRevision) + w.SetName(existingWorkload.GetName()) + return nil } // if revisionEnabled and the running workload's revision isn't equal to the component's latest reversion, // use revisionName as the workload name - if err := pv.SetString(instanceNamePath, componentLastRevision); err != nil { - return errors.Wrapf(err, errSetValueForField, instanceNamePath, c.Status.LatestRevision) - } - + w.SetName(componentLastRevision) return nil } // use component name as workload name, which means we will always use one workload for different revisions - if err := pv.SetString(instanceNamePath, c.GetName()); err != nil { - return errors.Wrapf(err, errSetValueForField, instanceNamePath, c.GetName()) - } - w.Object = pv.UnstructuredContent() + w.SetName(c.GetName()) return nil } @@ -342,6 +322,30 @@ func isRevisionEnabled(traitDefs []v1alpha2.TraitDefinition) bool { return false } +// setAppWorkloadInstanceName sets the name of the workload instance depends on the component revision +// and the workload kind +func setAppWorkloadInstanceName(componentName string, w *unstructured.Unstructured, revision int) { + // TODO: we can get the workloadDefinition name from w.GetLabels()["oam.WorkloadTypeLabel"] + // and use a special field like "use-inplace-upgrade" in the definition to allow configurable behavior + + // we hard code the behavior depends on the workload group/kind for now. The only in-place upgradable resources + // we support is cloneset for now. We can easily add more later. + if w.GroupVersionKind().Group == kruise.GroupVersion.Group { + if w.GetKind() == reflect.TypeOf(kruise.CloneSet{}).Name() { + // we use the component name alone for those resources that do support in-place upgrade + klog.InfoS("we reuse the component name for resources that support in-place upgrade", + "kind", w.GetKind(), "instance name", componentName) + w.SetName(componentName) + return + } + } + // we assume that the rest of the resources do not support in-place upgrade + instanceName := utils.ConstructRevisionName(componentName, int64(revision)) + klog.InfoS("we encountered an unknown resources, assume that it does not support in-place upgrade", + "kind", w.GetKind(), "instance name", instanceName) + w.SetName(instanceName) +} + // A ResourceRenderer renders a Kubernetes-compliant YAML resource into an // Unstructured object, optionally setting the supplied parameters. type ResourceRenderer interface { @@ -743,6 +747,16 @@ func (r *components) getDataInput(ctx context.Context, s *dagSource, ac *unstruc return rawval, true, "", nil } +func isControlledByApp(ac *v1alpha2.ApplicationConfiguration) bool { + for _, owner := range ac.GetOwnerReferences() { + if owner.APIVersion == v1alpha2.SchemeGroupVersion.String() && owner.Kind == v1alpha2.ApplicationKind && + owner.Controller != nil && *owner.Controller { + return true + } + } + return false +} + 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 { @@ -815,13 +829,9 @@ func checkConditions(conds []v1alpha2.ConditionRequirement, paved *fieldpath.Pav // GetTraitName return trait name func getTraitName(ac *v1alpha2.ApplicationConfiguration, componentName string, ct *v1alpha2.ComponentTrait, t *unstructured.Unstructured, traitDef *v1alpha2.TraitDefinition) string { - var ( - traitName string - apiVersion string - kind string - ) - - if len(t.GetName()) > 0 { + var traitName, apiVersion, kind string + // we forbid the trait name in the template if the applicationConfiguration is generated by application + if len(t.GetName()) > 0 && !isControlledByApp(ac) { return t.GetName() } diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go index fd9248b37..7db0c4a8a 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go @@ -35,11 +35,11 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" core "github.com/oam-dev/kubevela/apis/core.oam.dev" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" - "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/mock" "github.com/oam-dev/kubevela/pkg/oam/util" @@ -56,8 +56,8 @@ func TestRenderComponents(t *testing.T) { componentName := "coolcomponent" workloadName := "coolworkload" traitName := "coolTrait" - revisionName := "coolcomponent-aa1111" - revisionName2 := "coolcomponent-bb2222" + revisionName := "coolcomponent-v1" + revisionName2 := "coolcomponent-v2" ac := &v1alpha2.ApplicationConfiguration{ ObjectMeta: metav1.ObjectMeta{ @@ -74,6 +74,7 @@ func TestRenderComponents(t *testing.T) { }, }, } + revAC := &v1alpha2.ApplicationConfiguration{ ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, @@ -89,6 +90,17 @@ func TestRenderComponents(t *testing.T) { }, }, } + + controlledAC := revAC.DeepCopy() + controlledAC.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: v1alpha2.SchemeGroupVersion.String(), + Kind: v1alpha2.ApplicationKind, + Controller: pointer.BoolPtr(true), + }, + } + controlledAC.Spec.Components[0].RevisionName = revisionName2 + ref := metav1.NewControllerRef(ac, v1alpha2.ApplicationConfigurationGroupVersionKind) errTrait := errors.New("errTrait") @@ -525,6 +537,90 @@ func TestRenderComponents(t *testing.T) { }, }, }, + "Success-With-AppControlledAppConfig": { + reason: "Workload name should be component name for CloneSet", + fields: fields{ + client: &test.MockClient{MockGet: test.NewMockGetFn(nil, func(obj runtime.Object) error { + switch defObj := obj.(type) { + case *v1alpha2.Component: + ccomp := v1alpha2.Component{ + Status: v1alpha2.ComponentStatus{ + LatestRevision: &v1alpha2.Revision{Name: revisionName2}, + }, + } + ccomp.DeepCopyInto(defObj) + case *v1alpha2.TraitDefinition: + ttrait := v1alpha2.TraitDefinition{ObjectMeta: metav1.ObjectMeta{Name: traitName}, + Spec: v1alpha2.TraitDefinitionSpec{RevisionEnabled: true}} + ttrait.DeepCopyInto(defObj) + case *v1.ControllerRevision: + rev := &v1.ControllerRevision{ + ObjectMeta: metav1.ObjectMeta{Name: revisionName, Namespace: namespace}, + Data: runtime.RawExtension{Object: &v1alpha2.Component{ + ObjectMeta: metav1.ObjectMeta{ + Name: componentName, + Namespace: namespace, + }, + Spec: v1alpha2.ComponentSpec{ + Workload: runtime.RawExtension{ + Object: &unstructured.Unstructured{}, + }, + }, + Status: v1alpha2.ComponentStatus{ + LatestRevision: &v1alpha2.Revision{Name: revisionName2}, + }, + }}, + Revision: 2, + } + rev.DeepCopyInto(defObj) + } + return nil + })}, + params: ParameterResolveFn(func(_ []v1alpha2.ComponentParameter, _ []v1alpha2.ComponentParameterValue) ([]Parameter, error) { + return nil, nil + }), + workload: ResourceRenderFn(func(_ []byte, _ ...Parameter) (*unstructured.Unstructured, error) { + w := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps.kruise.io/v1alpha1", + "kind": "CloneSet", + }, + } + return w, nil + }), + trait: ResourceRenderFn(func(_ []byte, _ ...Parameter) (*unstructured.Unstructured, error) { + t := &unstructured.Unstructured{} + t.SetName(traitName) + return t, nil + }), + }, + args: args{ac: controlledAC}, + want: want{ + w: []Workload{ + { + ComponentName: componentName, + ComponentRevisionName: revisionName2, + Workload: func() *unstructured.Unstructured { + w := &unstructured.Unstructured{} + w.SetNamespace(namespace) + w.SetName(componentName) + w.SetOwnerReferences([]metav1.OwnerReference{*ref}) + w.SetAnnotations(map[string]string{ + oam.AnnotationAppGeneration: "0", + }) + w.SetLabels(map[string]string{ + oam.LabelAppComponent: componentName, + oam.LabelAppName: acName, + oam.LabelAppComponentRevision: revisionName2, + oam.LabelOAMResourceType: oam.ResourceTypeWorkload, + }) + return w + }(), + RevisionEnabled: true, + }, + }, + }, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -534,9 +630,16 @@ func TestRenderComponents(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nr.Render(...): -want error, +got error:\n%s\n", tc.reason, diff) } - if diff := cmp.Diff(tc.want.w, got); diff != "" { - t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) + if isControlledByApp(tc.args.ac) { + if diff := cmp.Diff(tc.want.w[0].Workload.GetName(), got[0].Workload.GetName()); diff != "" { + t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) + } + } else { + if diff := cmp.Diff(tc.want.w, got); diff != "" { + t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) + } } + }) } } @@ -1009,33 +1112,14 @@ func TestSetWorkloadInstanceName(t *testing.T) { }}, reason: "workloadName should align with componentName", }, - "set value error": { - u: &unstructured.Unstructured{Object: map[string]interface{}{ - "metadata": []string{}, - }}, - c: &v1alpha2.Component{ObjectMeta: metav1.ObjectMeta{Name: "comp"}, Status: v1alpha2.ComponentStatus{LatestRevision: &v1alpha2.Revision{Name: "rev-1"}}}, - expErr: errors.Wrapf(errors.New("metadata is not an object"), errSetValueForField, instanceNamePath, "comp"), - }, - "set value error for trait revisionEnabled": { - u: &unstructured.Unstructured{Object: map[string]interface{}{ - "metadata": []string{}, - }}, - traitDefs: []v1alpha2.TraitDefinition{ - { - Spec: v1alpha2.TraitDefinitionSpec{RevisionEnabled: false}, - }, - }, - c: &v1alpha2.Component{ObjectMeta: metav1.ObjectMeta{Name: "comp"}, Status: v1alpha2.ComponentStatus{LatestRevision: &v1alpha2.Revision{Name: "rev-1"}}}, - expErr: errors.Wrapf(errors.New("metadata is not an object"), errSetValueForField, instanceNamePath, "comp"), - }, } for name, ti := range tests { t.Run(name, func(t *testing.T) { if ti.expErr != nil { - assert.Equal(t, ti.expErr.Error(), SetWorkloadInstanceName(ti.traitDefs, ti.u, ti.c, + assert.Equal(t, ti.expErr.Error(), setWorkloadInstanceName(ti.traitDefs, ti.u, ti.c, ti.currentWorkload).Error()) } else { - err := SetWorkloadInstanceName(ti.traitDefs, ti.u, ti.c, ti.currentWorkload) + err := setWorkloadInstanceName(ti.traitDefs, ti.u, ti.c, ti.currentWorkload) assert.NoError(t, err) assert.Equal(t, ti.exp, ti.u, ti.reason) } @@ -1043,6 +1127,99 @@ func TestSetWorkloadInstanceName(t *testing.T) { } } +func TestSetAppWorkloadInstanceName(t *testing.T) { + tests := map[string]struct { + compName string + w *unstructured.Unstructured + revision int + expName string + reason string + }{ + "two resources case": { + compName: "webservice", + revision: 5, + w: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "extensions/v1beta1", + "kind": "deployment", + }}, + expName: "webservice-v5", + reason: "workloadName should be the component with revision", + }, + "one resources case": { + compName: "mysql", + revision: 2, + w: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "apps.kruise.io/v1alpha1", + "kind": "CloneSet", + }}, + expName: "mysql", + reason: "workloadName should be just the component name if we can do in-place upgrade", + }, + "ignore any existing name": { + compName: "mysql", + revision: 2, + w: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "apps.kruise.io/v1alpha1", + "kind": "CloneSet", + "metadata": map[string]interface{}{ + "name": "mysql-v1", + }, + }}, + expName: "mysql", + reason: "workloadName set in the template is ignored", + }, + "one resources same name case": { + compName: "mysql", + revision: 2, + w: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "oam.dev/v1alpha1", + "kind": "CloneSet", + }}, + expName: "mysql-v2", + reason: "we compare not only the kind but also the group name", + }, + } + for name, ti := range tests { + t.Run(name, func(t *testing.T) { + setAppWorkloadInstanceName(ti.compName, ti.w, ti.revision) + assert.Equal(t, ti.expName, ti.w.GetName(), ti.reason) + }) + } +} + +func TestIsControlledByApp(t *testing.T) { + // not true even if the kind checks right + ac := &v1alpha2.ApplicationConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "acName", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "api", + Kind: v1alpha2.ApplicationKind, + }, + }, + }, + } + assert.False(t, isControlledByApp(ac)) + // not true even the owner type checks right + ac.OwnerReferences = append(ac.OwnerReferences, metav1.OwnerReference{ + APIVersion: v1alpha2.SchemeGroupVersion.String(), + Kind: v1alpha2.ApplicationKind, + }) + assert.False(t, isControlledByApp(ac)) + // only true when it's the controller + ac.OwnerReferences[1].Controller = pointer.BoolPtr(true) + assert.True(t, isControlledByApp(ac)) + // still true when it's not the only the controller + ac.OwnerReferences = append(ac.OwnerReferences, metav1.OwnerReference{ + APIVersion: v1alpha2.SchemeGroupVersion.String(), + Kind: v1alpha2.ApplicationDeploymentKind, + Controller: pointer.BoolPtr(true), + }) + assert.True(t, isControlledByApp(ac)) +} + func TestSetTraitProperties(t *testing.T) { u := &unstructured.Unstructured{} u.SetName("hasName")