Feat: vela dry-run render results should be affected by override policy and deploy workflowstep #4815

cezhang · 2022-09-30T07:43:11Z

Feat: vela dry-run render results should be affected by override policy and deploy workflowstep

Signed-off-by: cezhang c1zhang.dev@gmail.com

Description of your changes

Fixes #4410

I have:

Read and followed KubeVela's contribution process.
Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Special notes for your reviewer

Example:
Input application

apiVersion: core.oam.dev/v1beta1
kind: Application
metadata:
  name: first-vela-app
spec:
  components:
    - name: express-server
      type: webservice
      properties:
        image: oamdev/hello-world
        ports:
         - port: 8000
           expose: true
      traits:
        - type: scaler
          properties:
            replicas: 1
  policies:
    - name: target-default
      type: topology
      properties:
        # The cluster with name local is installed the KubeVela.
        clusters: ["local"]
        namespace: "default"
    - name: target-prod
      type: topology
      properties:
        clusters: ["local"]
        # This namespace must be created before deploying.
        namespace: "prod"
    - name: deploy-ha
      type: override
      properties:
        components:
          - type: webservice
            traits:
              - type: scaler
                properties:
                  replicas: 3
  workflow:
    steps:
      - name: deploy2default
        type: deploy
        properties:
          policies: ["target-default"]
      - name: manual-approval
        type: suspend
      - name: deploy2prod
        type: deploy
        properties:
          policies: ["target-prod", "deploy-ha"]

Result from dryrun command considering override policy:

---
# Application(first-vela-app) -- Component(express-server) 
---

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations: {}
  labels:
    app.oam.dev/appRevision: ""
    app.oam.dev/component: express-server
    app.oam.dev/name: first-vela-app
    app.oam.dev/namespace: default
    app.oam.dev/resourceType: WORKLOAD
    workload.oam.dev/type: webservice
  name: express-server
  namespace: default
spec:
  replicas: 1
  selector:
    matchLabels:
      app.oam.dev/component: express-server
  template:
    metadata:
      labels:
        app.oam.dev/component: express-server
        app.oam.dev/name: first-vela-app
    spec:
      containers:
      - image: oamdev/hello-world
        name: express-server
        ports:
        - containerPort: 8000
          name: port-8000
          protocol: TCP

---
## From the auxiliary workload 
apiVersion: v1
kind: Service
metadata:
  annotations: {}
  labels:
    app.oam.dev/appRevision: ""
    app.oam.dev/component: express-server
    app.oam.dev/name: first-vela-app
    app.oam.dev/namespace: default
    app.oam.dev/resourceType: TRAIT
    trait.oam.dev/resource: webserviceExpose
    trait.oam.dev/type: AuxiliaryWorkload
  name: express-server
  namespace: default
spec:
  ports:
  - name: port-8000
    port: 8000
    targetPort: 8000
  selector:
    app.oam.dev/component: express-server
  type: ClusterIP

---

===
# The above components override by Policy(deploy-ha) below 
===

---
# Application(first-vela-app) -- Component(express-server) 
---

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations: {}
  labels:
    app.oam.dev/appRevision: ""
    app.oam.dev/component: express-server
    app.oam.dev/name: first-vela-app
    app.oam.dev/namespace: default
    app.oam.dev/resourceType: WORKLOAD
    workload.oam.dev/type: webservice
  name: express-server
  namespace: default
spec:
  replicas: 3
  selector:
    matchLabels:
      app.oam.dev/component: express-server
  template:
    metadata:
      labels:
        app.oam.dev/component: express-server
        app.oam.dev/name: first-vela-app
    spec:
      containers:
      - image: oamdev/hello-world
        name: express-server
        ports:
        - containerPort: 8000
          name: port-8000
          protocol: TCP

---
## From the auxiliary workload 
apiVersion: v1
kind: Service
metadata:
  annotations: {}
  labels:
    app.oam.dev/appRevision: ""
    app.oam.dev/component: express-server
    app.oam.dev/name: first-vela-app
    app.oam.dev/namespace: default
    app.oam.dev/resourceType: TRAIT
    trait.oam.dev/resource: webserviceExpose
    trait.oam.dev/type: AuxiliaryWorkload
  name: express-server
  namespace: default
spec:
  ports:
  - name: port-8000
    port: 8000
    targetPort: 8000
  selector:
    app.oam.dev/component: express-server
  type: ClusterIP

---

codecov · 2022-09-30T07:51:53Z

Codecov Report

Base: 61.21% // Head: 61.11% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (a7cae69) compared to base (360c9e2).
Patch coverage: 53.60% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4815      +/-   ##
==========================================
- Coverage   61.21%   61.11%   -0.11%     
==========================================
  Files         307      308       +1     
  Lines       46350    46659     +309     
==========================================
+ Hits        28375    28517     +142     
- Misses      15063    15180     +117     
- Partials     2912     2962      +50

Flag	Coverage Δ
apiserver-e2etests	`32.39% <0.00%> (-3.21%)`	⬇️
apiserver-unittests	`36.18% <ø> (-0.13%)`	⬇️
core-unittests	`55.25% <53.60%> (+0.05%)`	⬆️
e2e-multicluster-test	`18.95% <0.00%> (-0.05%)`	⬇️
e2e-rollout-tests	`20.73% <ø> (?)`
e2etests	`26.40% <ø> (+0.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/appfile/dryrun/dryrun.go	`55.23% <53.60%> (+10.99%)`	⬆️
pkg/utils/util/cmd.go	`20.00% <0.00%> (-13.34%)`	⬇️
pkg/addon/registry.go	`38.65% <0.00%> (-12.61%)`	⬇️
pkg/apiserver/event/sync/cr2ux.go	`50.00% <0.00%> (-10.47%)`	⬇️
...es/policydefinition/policydefinition_controller.go	`69.51% <0.00%> (-7.32%)`	⬇️
...aits/traitdefinition/traitdefinition_controller.go	`69.41% <0.00%> (-7.06%)`	⬇️
pkg/apiserver/domain/service/target.go	`57.86% <0.00%> (-3.38%)`	⬇️
pkg/cue/definition/template.go	`66.46% <0.00%> (-2.70%)`	⬇️
...troller/core.oam.dev/v1alpha2/application/apply.go	`87.02% <0.00%> (-1.90%)`	⬇️
pkg/apiserver/event/sync/cache.go	`73.46% <0.00%> (-1.54%)`	⬇️
... and 36 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Somefive

Seems you are making one copy of dryrun result for each override policy. There are some problems for that.

Override policy can be used jointly. Therefore, it is possible to use multiple override policies concurrently in one deploy step. This scenario is not taken into consideration AFAIS.
Declared override policy might not be used in workflow which means the dryrun result will not be actually deployed possibly.
Override policy can be referenced from the standalone override policy outside the application, it depends on how workflow refers to it.

Generally, I think the PR is a good attempt to tackle the difficult dynamic dryrun problem. But I recommend to consider the workflow deploy more carefully. Otherwise, this feature will cause confusions for users (since they will get different results between dryrun and real runs).

Somefive · 2022-09-30T08:53:14Z

pkg/appfile/dryrun/diff_test.go

 		ctx := context.Background()
-		Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "vela-system"}})).Should(Succeed())
+		//Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "vela-system"}})).Should(Succeed())


Is there any reason for commenting them? You can remove them if they are not in use anymore.

already move to suite_test BeforeSuite function, my testcase needs this setting as well. Cannot guarantee which testcase invoked first, if both testcase have this setting, error show.

Somefive · 2022-09-30T08:53:29Z

pkg/appfile/dryrun/diff_test.go

@@ -19,13 +19,9 @@ package dryrun
 import (
 	"bytes"
 	"context"
-	"io/ioutil"
-
+	types2 "github.com/oam-dev/kubevela/apis/types"


suggest using apitypes to replace types2

Somefive · 2022-09-30T08:57:16Z

references/cli/dryrun_test.go

+		//webserviceYAML := strings.Replace(string(webservice), "{{ include \"systemDefinitionNamespace\" . }}", types.DefaultKubeVelaNS, 1)
+		//wwd := v1beta1.ComponentDefinition{}
+		//Expect(yaml.Unmarshal([]byte(webserviceYAML), &wwd)).Should(BeNil())
+		//Expect(k8sClient.Create(context.TODO(), &wwd)).Should(BeNil())


You can just remove them, I think.

Somefive · 2022-09-30T08:57:58Z

references/cli/dryrun_test.go

 	"sigs.k8s.io/yaml"

+	"github.com/oam-dev/kubevela/pkg/utils/util"
+


remove this line and reformat the imports

Somefive · 2022-09-30T09:02:05Z

pkg/appfile/dryrun/dryrun.go

@@ -206,3 +210,33 @@ func (d *Option) PrintDryRun(buff *bytes.Buffer, appName string, comps []*types.
 	}
 	return nil
 }
+
+// ExecuteDryRunWithOverridePolicy is similar to ExecuteDryRun function, but patch components with override policy
+func (d *Option) ExecuteDryRunWithOverridePolicy(ctx context.Context, application *v1beta1.Application) (map[string][]*types.ComponentManifest, error) {


Is this function aware of deploy step?

if not using offline, yes

cezhang · 2022-09-30T14:16:06Z

For the 3rd point, could u share a example?

Somefive · 2022-09-30T15:02:23Z

For the 3rd point, could u share a example?

https://kubevela.net/docs/end-user/components/ref-objects#distribute-reference-objects-with-different-configuration

StevenLeiZhang · 2022-10-29T08:31:20Z

pkg/appfile/dryrun/diff_test.go

-			un.SetNamespace(ns)
-			Expect(k8sClient.Create(context.Background(), un)).Should(Succeed())
-		}
+		//applyFile := func(filename string, ns string) {


delete the useless codes

StevenLeiZhang · 2022-10-29T08:34:43Z

pkg/appfile/dryrun/dryrun.go

+// ExecuteDryRunWithOverridePolicy is similar to ExecuteDryRun function, but patch components with override policy
+func (d *Option) ExecuteDryRunWithOverridePolicy(ctx context.Context, application *v1beta1.Application) (map[string][]*types.ComponentManifest, error) {
+
+	compsForEechOverridePolicy := make(map[string][]*types.ComponentManifest)


Is "Eech" one typo? I guess you want "Each".

I am re-implementing this feature, btw, thanks for the head-ups

StevenLeiZhang · 2022-10-29T08:38:25Z

pkg/appfile/dryrun/dryrun.go

+
+	compsForEechOverridePolicy := make(map[string][]*types.ComponentManifest)
+	for _, policy := range application.Spec.Policies {
+		if policy.Type == v1alpha1.OverridePolicyType {


if policy.Type != v1alpha1.OverridePolicyType { continue } /your main logic here/

Then you get less indent.

…policy and deploy workflowstep Signed-off-by: cezhang <c1zhang.dev@gmail.com>

cezhang · 2022-12-16T09:31:13Z

@Somefive @wonderflow please help take a quick review on the new impl

wonderflow · 2022-12-16T10:06:33Z

@cezhang What's the current behavior? Can you provide some examples that aligns with the design: #5184 (reply in thread)

Somefive · 2022-12-20T06:58:32Z

Great impl. Several suggestions for the change:

Let's reuse the -f parameter and let it accepts multiple file inputs, like -f a.yaml,b.yaml,c.yaml and -f a.yaml -f b.yaml, instead of adding the -p parameter. The PolicyFiles are not necessarily be treated differently.
[Optional] Report error if multiple applications are found or no application found. Report error when resources other than Application/Policy/Workflow are included. Report error when multiple Workflows are provided. Report warnings if the external policy is not used in deploy workflow step.
[Optional] Support workflow file input.
[Optional] Support --merge to make external policy take effect when no workflow specified.

2~4 are recommended but we can leave their implementations for the future.

…e orphan policy or workflow Signed-off-by: cezhang <c1zhang.dev@gmail.com>

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

Somefive

Generally LGTM after minor fixes, great job!

references/cli/dryrun_test.go

Somefive · 2023-01-09T06:04:46Z

references/cli/dryrun.go

+
+		fileType := filepath.Ext(filename)
+		switch fileType {
+		case ".yaml", ".yml":


only one case for fileType?

Somefive · 2023-01-09T06:05:52Z

references/cli/dryrun.go

+		switch fileType {
+		case ".yaml", ".yml":
+			// only support one object in one yaml file
+			fileContent, err = yaml.YAMLToJSON(fileContent)


So we only support one object in one yaml file?

yes, only handle one object per file for now(yaml format, actually). Plan to add more support from up coming improvement

Great, I strongly suggest supporting multiple files here. Although it is not necessary to be contained in this PR. And the multiple file loading could be moved into kubevela/pkg and be reused across various projects.

references/cli/dryrun.go

pkg/appfile/dryrun/diff_test.go

references/cli/dryrun.go

pkg/appfile/dryrun/diff_test.go

wonderflow · 2023-01-09T06:39:54Z

references/cli/dryrun.go

+	ApplicationFiles []string
+	DefinitionFile   string
+	OfflineMode      bool
+	MergeOrphanFiles bool
 }

 // NewDryRunCommand creates `dry-run` command


let's add some explanation about the working mechanism.

Add some examples commands.

Dry Run with policy and workflow will only take override/topology policies and deploy workflow step into considerations. Other workflow step will be ignored.

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

Somefive

LGTM, great job!

wonderflow

LGTM, please fix the CI

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

github-actions · 2023-01-11T05:53:12Z

Backport failed for release-1.7, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-1.7
git worktree add -d .worktree/backport-4815-to-release-1.7 origin/release-1.7
cd .worktree/backport-4815-to-release-1.7
git checkout -b backport-4815-to-release-1.7
ancref=$(git merge-base b71a8a353ac5a9357819b9a5bb55b4671feb8b05 a7cae693883b4fd24122611afe88d66c1cb12002)
git cherry-pick -x $ancref..a7cae693883b4fd24122611afe88d66c1cb12002

…cy and deploy workflowstep (kubevela#4815) * [Feature] vela dry-run render results should be affected by override policy and deploy workflowstep Signed-off-by: cezhang <c1zhang.dev@gmail.com> * multiple input files support; policy,workflow support; new flag: merge orphan policy or workflow Signed-off-by: cezhang <c1zhang.dev@gmail.com> * add more tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix comment issues Signed-off-by: cezhang <c1zhang.dev@gmail.com> * add tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix e2e Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> Signed-off-by: cezhang <c1zhang.dev@gmail.com>

…cy and deploy workflowstep (#4815) (#5314) * [Feature] vela dry-run render results should be affected by override policy and deploy workflowstep Signed-off-by: cezhang <c1zhang.dev@gmail.com> * multiple input files support; policy,workflow support; new flag: merge orphan policy or workflow Signed-off-by: cezhang <c1zhang.dev@gmail.com> * add more tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix comment issues Signed-off-by: cezhang <c1zhang.dev@gmail.com> * add tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix e2e Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> Signed-off-by: cezhang <c1zhang.dev@gmail.com> Signed-off-by: cezhang <c1zhang.dev@gmail.com> Co-authored-by: cezhang <c1zhang.dev@gmail.com>

…cy and deploy workflowstep (kubevela#4815) * [Feature] vela dry-run render results should be affected by override policy and deploy workflowstep Signed-off-by: cezhang <c1zhang.dev@gmail.com> * multiple input files support; policy,workflow support; new flag: merge orphan policy or workflow Signed-off-by: cezhang <c1zhang.dev@gmail.com> * add more tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix comment issues Signed-off-by: cezhang <c1zhang.dev@gmail.com> * add tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix e2e Signed-off-by: cezhang <c1zhang.dev@gmail.com> * fix tests Signed-off-by: cezhang <c1zhang.dev@gmail.com> Signed-off-by: cezhang <c1zhang.dev@gmail.com>

cezhang requested review from Somefive, zzxwill, StevenLeiZhang, charlie0129, chivalryq, barnettZQG, wonderflow, leejanee and jefree-cat as code owners September 30, 2022 07:43

cezhang changed the title ~~Feat: vela dry-run render results should be affected by override poli…~~ Feat: vela dry-run render results should be affected by override policy and deploy workflowstep Sep 30, 2022

Somefive reviewed Sep 30, 2022

View reviewed changes

StevenLeiZhang reviewed Oct 29, 2022

View reviewed changes

[Feature] vela dry-run render results should be affected by override …

ac73bef

…policy and deploy workflowstep Signed-off-by: cezhang <c1zhang.dev@gmail.com>

cezhang force-pushed the #4410 branch from 1a752a1 to c0a9bd9 Compare December 16, 2022 09:22

cezhang requested a review from FogDong as a code owner December 16, 2022 09:22

cezhang force-pushed the #4410 branch from c0a9bd9 to ac73bef Compare December 16, 2022 09:28

cezhang added 3 commits January 5, 2023 12:45

multiple input files support; policy,workflow support; new flag: merg…

3ff71d9

…e orphan policy or workflow Signed-off-by: cezhang <c1zhang.dev@gmail.com>

Merge branch 'master' into kubevela#4410

1869ae8

add more tests

3496c35

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

Somefive reviewed Jan 9, 2023

View reviewed changes

wonderflow added the backport release-1.7 add this label will automatically backport this PR to release-1.7 branch label Jan 9, 2023

wonderflow reviewed Jan 9, 2023

View reviewed changes

references/cli/dryrun.go Outdated Show resolved Hide resolved

wonderflow reviewed Jan 9, 2023

View reviewed changes

pkg/appfile/dryrun/diff_test.go Outdated Show resolved Hide resolved

wonderflow reviewed Jan 9, 2023

View reviewed changes

fix comment issues

cc43d37

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

cezhang requested review from wonderflow and Somefive and removed request for wonderflow and Somefive January 9, 2023 08:48

Somefive approved these changes Jan 10, 2023

View reviewed changes

wonderflow approved these changes Jan 10, 2023

View reviewed changes

add tests

e7eec64

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

cezhang force-pushed the #4410 branch from 4022024 to e7eec64 Compare January 10, 2023 10:21

cezhang added 2 commits January 11, 2023 10:32

fix e2e

b44269e

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

fix tests

a7cae69

Signed-off-by: cezhang <c1zhang.dev@gmail.com>

wonderflow merged commit 1ce5c6d into kubevela:master Jan 11, 2023

wonderflow mentioned this pull request Jan 11, 2023

Feat: vela dry-run render results should be affected by override policy and deploy workflowstep (backport #4815) #5314

Merged

4 tasks

cezhang deleted the #4410 branch January 11, 2023 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: vela dry-run render results should be affected by override policy and deploy workflowstep #4815

Feat: vela dry-run render results should be affected by override policy and deploy workflowstep #4815

cezhang commented Sep 30, 2022

codecov bot commented Sep 30, 2022 •

edited

Somefive left a comment

Somefive Sep 30, 2022

cezhang Sep 30, 2022

Somefive Sep 30, 2022

Somefive Sep 30, 2022

Somefive Sep 30, 2022

Somefive Sep 30, 2022

cezhang Sep 30, 2022

cezhang commented Sep 30, 2022

Somefive commented Sep 30, 2022

StevenLeiZhang Oct 29, 2022

StevenLeiZhang Oct 29, 2022

cezhang Oct 31, 2022

StevenLeiZhang Oct 29, 2022

cezhang commented Dec 16, 2022

wonderflow commented Dec 16, 2022

Somefive commented Dec 20, 2022

Somefive left a comment

Somefive Jan 9, 2023

Somefive Jan 9, 2023

cezhang Jan 9, 2023

Somefive Jan 10, 2023

wonderflow Jan 9, 2023

Somefive left a comment

wonderflow left a comment

github-actions bot commented Jan 11, 2023

		"sigs.k8s.io/yaml"

		"github.com/oam-dev/kubevela/pkg/utils/util"

Feat: vela dry-run render results should be affected by override policy and deploy workflowstep #4815

Feat: vela dry-run render results should be affected by override policy and deploy workflowstep #4815

Conversation

cezhang commented Sep 30, 2022

Description of your changes

How has this code been tested

Special notes for your reviewer

codecov bot commented Sep 30, 2022 • edited

Codecov Report

Somefive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezhang commented Sep 30, 2022

Somefive commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezhang commented Dec 16, 2022

wonderflow commented Dec 16, 2022

Somefive commented Dec 20, 2022

Somefive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Somefive left a comment

Choose a reason for hiding this comment

wonderflow left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2023

codecov bot commented Sep 30, 2022 •

edited