feat(backend): workflow validation. Fixes #3526. #3965

NikeNano · 2020-06-11T12:08:01Z

Currently pipelines will not be validated until run, this result in that it takes longer time to test and upload pipelines. This PR(current draft) aims to fix this and resolved #3526.

The goal is to validate pipelines when uploaded and then return the errors.

kubeflow-bot · 2020-06-11T12:08:08Z

This change is

Ark-kun · 2020-06-12T03:50:02Z

Thanks for this work!

BTW, the SDK performs the validation if working argo is in path. But it's still better for backend to validate.

Th only issue I see is that we need to be able to update the backend Argo module when updating the manifests.

NikeNano · 2020-06-12T18:12:00Z

BTW, the SDK performs the validation if working argo is in path. But it's still better for backend to validate.

Ohh is it this work you relate to @Ark-kun ?

backend/src/apiserver/resource/resource_manager.go

backend/src/common/util/template_util.go

NikeNano · 2020-06-15T11:33:06Z

I am rather new to Golang (learning through opensource) and dont now how to fix this issue with currently breaks the tests.

Known dependencies are:
	github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow/v1beta1
	github.com/argoproj/argo/pkg/apis/workflow/v1alpha1
	github.com/ghodss/yaml
	github.com/stretchr/testify/assert
	k8s.io/apimachinery/pkg/api/errors
	k8s.io/apimachinery/pkg/apis/meta/v1
	k8s.io/apimachinery/pkg/runtime/schema
	k8s.io/apimachinery/pkg/types
	k8s.io/kubernetes/pkg/apis/core
	google.golang.org/grpc/codes
	github.com/kubeflow/pipelines/backend/api/go_client
	github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow
	github.com/cenkalti/backoff
	github.com/go-openapi/runtime
	github.com/go-openapi/strfmt
	github.com/golang/glog
	github.com/google/uuid
	github.com/pkg/errors
	github.com/golang/protobuf/ptypes/timestamp
	k8s.io/apimachinery/pkg/util/json
	k8s.io/client-go/kubernetes
	k8s.io/client-go/rest
	k8s.io/client-go/tools/clientcmd
	google.golang.org/grpc
	google.golang.org/grpc/status
Check that imports in Go sources match importpath attributes in deps.

I have added the dependency to the go.mod but it dont seem to help ... :( Is there anyone that could give some advice on how to solve this/where I can read up to solve it. @IronPan, @mgogogo, @Ark-kun Thanks for the help!

NikeNano · 2020-06-26T14:25:44Z

friendly ping @rmgogogo

Bobgy · 2020-06-29T03:51:55Z

You may ask @jingzhang36 or @IronPan.

rmgogogo · 2020-06-30T07:09:23Z

I'm general OK with this PR.

@jingzhang36 leave this to your LGTM

rmgogogo · 2020-06-30T07:11:51Z

I am rather new to Golang (learning through opensource) and dont now how to fix this issue with currently breaks the tests.

Known dependencies are:
	github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow/v1beta1
	github.com/argoproj/argo/pkg/apis/workflow/v1alpha1
	github.com/ghodss/yaml
	github.com/stretchr/testify/assert
	k8s.io/apimachinery/pkg/api/errors
	k8s.io/apimachinery/pkg/apis/meta/v1
	k8s.io/apimachinery/pkg/runtime/schema
	k8s.io/apimachinery/pkg/types
	k8s.io/kubernetes/pkg/apis/core
	google.golang.org/grpc/codes
	github.com/kubeflow/pipelines/backend/api/go_client
	github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow
	github.com/cenkalti/backoff
	github.com/go-openapi/runtime
	github.com/go-openapi/strfmt
	github.com/golang/glog
	github.com/google/uuid
	github.com/pkg/errors
	github.com/golang/protobuf/ptypes/timestamp
	k8s.io/apimachinery/pkg/util/json
	k8s.io/client-go/kubernetes
	k8s.io/client-go/rest
	k8s.io/client-go/tools/clientcmd
	google.golang.org/grpc
	google.golang.org/grpc/status
Check that imports in Go sources match importpath attributes in deps.

I have added the dependency to the go.mod but it dont seem to help ... :( Is there anyone that could give some advice on how to solve this/where I can read up to solve it. @IronPan, @mgogogo, @Ark-kun Thanks for the help!

How you update go.mod? Is it via manual?

Here is the suggested steps:

https://github.com/kubeflow/pipelines/tree/master/backend#updating-build-files

It's true that we didn't update it for a long time.

jingzhang36 · 2020-06-30T07:22:16Z

@NikeNano could you add the new dependency on github.com/argoproj/argo/workflow/validate to BUILD.bazel in this directory?

Bobgy · 2021-05-28T00:11:50Z

backend/src/apiserver/resource/resource_manager_test.go

 })

+func setMetadata(wf *v1alpha1.Workflow) {


nit: addRuntimeWorkflowMetadata? to make tests easier to understand

Probably we can omit workflow, addRuntimeMetadata? WDYT?

We can set it directly on the test workflow and drop the method to clean up.

Hi @NikeNano, note the current change is against the goal we want to test. In real world, the workflows passed from KFP compiler do not have these metadata annotations. Therefore, always adding them beforehand makes tests deviate from the real world.

I understand we all want tests to be simpler, so they can be more readable. I think a better way to do this is to stop comparing all fields of a workflow in tests. Instead, we can reset fields we do not care about in some tests to empty before comparing with expected workflow. e.g. for this case, we can simply set annotations and labels to empty, and do not include them in the expected workflow.

Note that, for tests that specifically verify labels & annotations are correct, we still keep them to test what we want.

Therefore, always adding them beforehand makes tests deviate from the real world.

Yes good point, will have to find another solution.

I see your point, the tricky part here is that the WorkflowRuntimeManifest is handled as a string. Which means we need to unmarshal it to a *v1alpha1.Workflow again, remove the metadata and convert to a string again and then set it on the runDetails. I think this adds more complexity than just adding it atm.

NikeNano · 2021-06-01T17:35:34Z

backend/src/apiserver/resource/resource_manager_test.go

@@ -208,6 +219,19 @@ func createPipeline(name string) *model.Pipeline {
 		}}
 }

+func createRunExpectedWorkflow(params []v1alpha1.Parameter, labels map[string]string, annotations map[string]string, serviceAccount string) *v1alpha1.Workflow {


How about this @Bobgy should make tests a bit cleaner? We still do the comparison with the metadata but as I mentioned here I think it might be easier for the reader if it is included in this case.

Do you think this is better than the original addMetadata version? I feel like that's simpler, because it sticks to workflow struct in interface.

I only had concerns regarding naming. Current implementation is good in a few minor points too, so overall I think the following are good:

The method returns a new workflow via deepcopy (so that the method does not mutate its inputs)

The method name makes tests readable -- we can tell it adds some metadata that is present on runtime workflows.

WDYT?

I feel sorry you had to change the entire test file multiple times, I think we can save some intermediate efforts by commenting ideas and discuss in comments, then finish the code when we agree with each other.

I rewrote to use createRunExpectedWorkflow since all the tests related to CreateRun have the same similar set up required where the metadata is a subset only. I don't have any strong feelings for either of the solutions, but maybe bringing back the addMetadata is easiest and then have a separate look on the tests to improve the readability?

Problem with the current createRunExpectedWorkflow interface is that, there are a fixed number of arguments.
Whenever any test calls it, it has to supply all the arguments. However, if we will need to add any new arguments in the future, we'll have to update every test that calls createRunExpectedWorkflow.

Also, it creates another layer of abstraction over directly creating workflow structs, but the new abstraction doesn't really provide much value, it only adds metadata by default.

Compared to this, the original addMetadata is a lot simpler. I was only asking for a more descriptive name.

Therefore, my proposal would be:

func withCreateRunMetadata(*v1alpha1.Workflow workflow) *v1alpha1.Workflow { workflow.DeepCopy() ... (you can adjust the name if you have better ideas) }

@NikeNano see above

Cool, see your point that it is a simpler interface, will update.

Bobgy

Thank you for the update!
/lgtm

There's only one minor comment left.

Bobgy · 2021-06-08T08:59:05Z

backend/src/apiserver/resource/resource_manager_test.go

@@ -208,6 +219,14 @@ func createPipeline(name string) *model.Pipeline {
 		}}
 }

+func withCreateRunMetadata(wf *v1alpha1.Workflow) *v1alpha1.Workflow {
+	template := wf.Spec.Templates[0]


nit: deepCopy the workflow before changing it?
You are now editing the input wf.

Another option is to not add the return value, and make it explicit this method changes input.

It's up-to-you to decide.

Went back to the addRuntimeMetadata and add the metadata to the input workflow, the circle is complete :)

NikeNano · 2021-06-08T20:51:40Z

backend/src/apiserver/server/run_server_test.go

+	template.Metadata.Annotations = map[string]string{"sidecar.istio.io/inject": "false"}
+	template.Metadata.Labels = map[string]string{"pipelines.kubeflow.org/cache_enabled": "true"}


Asking before I change too much again. In order to reuse the addRuntimeMetadata it needs to live outside of the test file(or be duplicated). What is best practice in this case? Should we just add test_util.go in order to make it exported?

Agree, we can add a test_util.go

Bobgy · 2021-06-14T00:34:44Z

/lgtm
/approve
Thank you so much for this great addition to KFP and also your perseverance while implementing and adapting the PR!
Really appreciate it

google-oss-robot · 2021-06-14T00:34:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from IronPan and rmgogogo June 11, 2020 12:08

k8s-ci-robot added size/XS size/S and removed size/XS labels Jun 11, 2020

NikeNano marked this pull request as draft June 11, 2020 13:18

k8s-ci-robot added the do-not-merge/work-in-progress label Jun 11, 2020

k8s-ci-robot added size/M and removed size/S labels Jun 12, 2020

NikeNano marked this pull request as ready for review June 14, 2020 12:16

k8s-ci-robot removed the do-not-merge/work-in-progress label Jun 14, 2020

NikeNano commented Jun 14, 2020

View reviewed changes

backend/src/apiserver/resource/resource_manager.go Outdated Show resolved Hide resolved

NikeNano commented Jun 14, 2020

View reviewed changes

backend/src/common/util/template_util.go Outdated Show resolved Hide resolved

NikeNano marked this pull request as draft June 15, 2020 06:50

k8s-ci-robot added the do-not-merge/work-in-progress label Jun 15, 2020

NikeNano marked this pull request as ready for review June 15, 2020 11:03

k8s-ci-robot removed the do-not-merge/work-in-progress label Jun 15, 2020

Ark-kun requested a review from jingzhang36 June 17, 2020 06:21

Ark-kun assigned rmgogogo and jingzhang36 Jun 17, 2020

Ark-kun added the area/backend label Jun 17, 2020

Bobgy reviewed May 28, 2021

View reviewed changes

NikeNano commented Jun 1, 2021

View reviewed changes

NikeNano force-pushed the workflow_validation branch from 5c89a12 to 3210e7f Compare June 7, 2021 20:35

Bobgy reviewed Jun 8, 2021

View reviewed changes

google-oss-robot assigned Bobgy Jun 8, 2021

google-oss-robot added lgtm and removed lgtm labels Jun 8, 2021

NikeNano added 14 commits June 8, 2021 21:13

Added validation to the workflow

acbacd8

check if template is empty

065e34d

remove if and add test that fails

3c90e2e

rework the code a bit

ea03509

fix backend test

085d9f9

renamed to clean up

59afa74

fix tests

cd7b759

clean up the tests

dddcbad

clean up and fix tests

553262d

clean up

9dc94d9

clean up tests

fc187db

update after feedback

d18b44a

back to addRuntimeMetadata

202322a

rebase with master

f246e9f

NikeNano force-pushed the workflow_validation branch from 383e14e to f246e9f Compare June 8, 2021 20:16

NikeNano commented Jun 8, 2021

View reviewed changes

NikeNano added 2 commits June 9, 2021 15:49

move addRuntimeMetadata to seperate file to be exported

fe30fca

Add the add metadata function

b610640

google-oss-robot added the lgtm label Jun 14, 2021

google-oss-robot added the approved label Jun 14, 2021

google-oss-robot merged commit a44a225 into kubeflow:master Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): workflow validation. Fixes #3526. #3965

feat(backend): workflow validation. Fixes #3526. #3965

NikeNano commented Jun 11, 2020

kubeflow-bot commented Jun 11, 2020

Ark-kun commented Jun 12, 2020

NikeNano commented Jun 12, 2020 •

edited

Loading

NikeNano commented Jun 15, 2020

NikeNano commented Jun 26, 2020

Bobgy commented Jun 29, 2020

rmgogogo commented Jun 30, 2020

rmgogogo commented Jun 30, 2020

jingzhang36 commented Jun 30, 2020

Bobgy May 28, 2021

Bobgy May 28, 2021

NikeNano May 28, 2021

Bobgy Jun 1, 2021

NikeNano Jun 1, 2021

NikeNano Jun 1, 2021

Bobgy Jun 1, 2021

Bobgy Jun 1, 2021

NikeNano Jun 3, 2021

Bobgy Jun 7, 2021

Bobgy Jun 7, 2021

NikeNano Jun 7, 2021

Bobgy left a comment

Bobgy Jun 8, 2021

NikeNano Jun 8, 2021 •

edited

Loading

NikeNano Jun 8, 2021

Bobgy Jun 9, 2021

Bobgy commented Jun 14, 2021

google-oss-robot commented Jun 14, 2021

		template.Metadata.Annotations = map[string]string{"sidecar.istio.io/inject": "false"}
		template.Metadata.Labels = map[string]string{"pipelines.kubeflow.org/cache_enabled": "true"}

feat(backend): workflow validation. Fixes #3526. #3965

feat(backend): workflow validation. Fixes #3526. #3965

Conversation

NikeNano commented Jun 11, 2020

kubeflow-bot commented Jun 11, 2020

Ark-kun commented Jun 12, 2020

NikeNano commented Jun 12, 2020 • edited Loading

NikeNano commented Jun 15, 2020

NikeNano commented Jun 26, 2020

Bobgy commented Jun 29, 2020

rmgogogo commented Jun 30, 2020

rmgogogo commented Jun 30, 2020

jingzhang36 commented Jun 30, 2020

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

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

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikeNano Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented Jun 14, 2021

google-oss-robot commented Jun 14, 2021

NikeNano commented Jun 12, 2020 •

edited

Loading

NikeNano Jun 8, 2021 •

edited

Loading