Merge pull request #1091 from ryanzhang-oss/render-workload-name

render the workload and trait name differently

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()
 	}
 

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")