Install application CRD and add pipeline application CR to pipeline standalone #2585

IronPan · 2019-11-09T01:20:59Z

Add application CRD and application manager
Add pipeline application CR
Set addOwnerRef=true for pipeline application CR so things will get GCed when application is deleted
Set a common label application-crd-id: kubeflow-pipelines for identifying the KFP components. This will be used as identifier by application manager to set ownerref. Note we also set it for application manager themselves as they will get GCed too.

Note we don't set the label for CRD or namespace since they are cluster-scoped resources and application CRD doesn't support such resources. Setting labels on those resources will leads to unexpected GC behavior.

This change is

IronPan · 2019-11-09T01:21:31Z

/assign @Bobgy

IronPan · 2019-11-09T01:25:44Z

fix #2469

Bobgy

Thanks! Looks great.
/lgtm

Left a few comments for clarification.

manifests/kustomize/base/kustomization.yaml

manifests/kustomize/base/argo/minio-artifact-secret.yaml

manifests/kustomize/base/pipeline-application.yaml

Bobgy · 2019-11-09T15:39:10Z

Also, note that there's an error in e2e tests when deploying these.

error: error validating "STDIN": error validating data: ValidationError(Deployment.spec): unknown field "serviceName" in io.k8s.api.apps.v1beta2.DeploymentSpec; if you choose to ignore these errors, turn validation off with --validate=false

IronPan · 2019-11-11T23:59:28Z

/test kubeflow-pipeline-e2e-test

Bobgy · 2019-11-12T00:49:08Z

manifests/kustomize/README.md

-export PIPELINE_VERSION=0.1.31
-kubectl apply -f https://storage.googleapis.com/ml-pipeline/pipeline-lite/$PIPELINE_VERSION/namespaced-install.yaml
+export PIPELINE_VERSION=0.1.34
+for i in {1..2}; do kubectl apply -f https://storage.googleapis.com/ml-pipeline/pipeline-lite/$PIPELINE_VERSION/namespaced-install.yaml && break || sleep 1; done


Can you help me understand why we need these retries in README?

Besides, shall we sync this file with documentation on kubeflow/website?

adding crd definition is a eventual consistent behavior. by adding crd and cr in the same command, somehow master always throw me

error: unable to recognize "STDIN": no matches for kind "Application" in version "app.k8s.io/v1beta1"

which is really pain. blind retry works so i added it here.

And regarding the doc, i can update once this is merged.

It might take a few seconds for the endpoint to be created. You can watch the Established condition of your CustomResourceDefinition to be true or watch the discovery information of the API server for your resource to show up.
https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#create-a-customresourcedefinition

I did some investigation, as you said, applying a CRD is eventual consistency (it takes some time before the k8s api endpoint is ready). So in reality it could take any amount of time before CRDs are ready. Using a retry here may work consistently now, but I think that's related to the time waited during retry just happen to be long enough for the CRD to be established. Any of these conditions may change in the future, e.g. if applying happens faster, or if CRDs take longer to establish, then this retry will break.

Also, everyone who read this the first time would wonder why we need a retry here.

I prefer we make the waiting explicit like the following. It is also pretty self explanatory about what we are doing.

kubectl apply -f crds kubectl wait --for condition=established --timeout=60s crd/applications.app.k8s.io kubectl apply -k envs/dev

https://stackoverflow.com/a/57126479

manifests/kustomize/base/pipeline-application.yaml

test/deploy-pipeline-lite.sh

rmgogogo · 2019-12-19T09:39:30Z

manifests/kustomize/base/pipeline-application.yaml

+    matchLabels:
+      application-crd-id: kubeflow-pipelines
+  descriptor:
+    version: 0.1.34


Quick check, will our on-call script auto update this field? not familiar with that part.
It's OK if we put that change to another PR.

We should update release playbook to also change this field

thanks. will do once this is merged.

Bobgy

I will further investigate the retry issue. Its behavior is strange.

Even if I delete everything from a cluster, second kubectl apply doesn't fail any more.

manifests/kustomize/base/crds/application-crd.yaml

Bobgy · 2019-12-23T10:10:54Z

manifests/kustomize/base/pipeline-application.yaml

+    matchLabels:
+      application-crd-id: kubeflow-pipelines
+  descriptor:
+    version: 0.1.34


We should update release playbook to also change this field

rmgogogo · 2020-01-09T09:22:43Z

kindly ping this PR.
I also prefer change to apply twice, one for CRD and then for others to avoid retry logic.

Bobgy

Thanks! The new changes look good. Some final minor comments.

/approve

Bobgy · 2020-01-16T05:30:17Z

manifests/kustomize/base/pipeline/ml-pipeline-viewer-crd-sa.yaml

@@ -1,4 +1,4 @@
 apiVersion: v1
 kind: ServiceAccount
 metadata:
-  name: ml-pipeline-viewer-crd-service-account
+  name: ml-pipeline-viewer-crd-service-account


nit: can you revert this change?

test/manifests/crd/kustomization.yaml

Bobgy · 2020-01-16T06:51:22Z

/lgtm

Bobgy · 2020-01-16T06:55:52Z

/lgtm

IronPan · 2020-01-16T07:25:03Z

/approve

k8s-ci-robot · 2020-01-16T07:25:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, IronPan

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~~ [IronPan]

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

…tandalone (kubeflow#2585) * install application CRD and add pipeline application CR * add labels and let application manager to set ownerref * fix * address comments * update test * update test * update test * update readme * fix test * update * update * update * Update application-crd.yaml * fix * fix * Update .release.cloudbuild.yaml * update tests * Update kustomization.yaml * Update deploy-pipeline-lite.sh * Update ml-pipeline-viewer-crd-sa.yaml * update tests * update tests * update tests

IronPan added 2 commits November 8, 2019 14:41

install application CRD and add pipeline application CR

fbc4efc

add labels and let application manager to set ownerref

68cc40e

k8s-ci-robot added the size/XL label Nov 9, 2019

k8s-ci-robot requested review from neuromage and paveldournov November 9, 2019 01:21

k8s-ci-robot assigned Bobgy Nov 9, 2019

fix

b5a96ce

Bobgy reviewed Nov 9, 2019

View reviewed changes

manifests/kustomize/base/kustomization.yaml Outdated Show resolved Hide resolved

manifests/kustomize/base/argo/minio-artifact-secret.yaml Outdated Show resolved Hide resolved

manifests/kustomize/base/pipeline-application.yaml Show resolved Hide resolved

k8s-ci-robot added the lgtm label Nov 9, 2019

address comments

e37db95

k8s-ci-robot added size/L and removed lgtm size/XL labels Nov 11, 2019

IronPan added 5 commits November 11, 2019 10:11

update test

a834d0e

update test

74d264e

update test

0d8759c

update readme

67c50f9

fix test

2eb224c

Bobgy reviewed Nov 12, 2019

View reviewed changes

rmgogogo self-assigned this Nov 14, 2019

IronPan added 5 commits December 16, 2019 09:36

update

ed4511d

update

4c422c0

update

c6c72a5

update

f94b954

Merge branch 'master' into app

82937f7

rmgogogo reviewed Dec 19, 2019

View reviewed changes

Bobgy reviewed Dec 23, 2019

View reviewed changes

IronPan and others added 2 commits January 6, 2020 16:38

Merge branch 'master' into app

a5a2b29

Update application-crd.yaml

4af4392

IronPan and others added 8 commits January 15, 2020 09:47

fix readme

f9763f1

fix readme

6b62b75

fix

8713dbb

fix

7b31b8f

Update .release.cloudbuild.yaml

3d4a658

update tests

082b50f

Merge branch 'app' of https://github.com/IronPan/pipelines into app

1b8deb8

Merge remote-tracking branch 'upstream/master' into app

eabd55d

Bobgy reviewed Jan 16, 2020

View reviewed changes

Bobgy added the area/deployment/standalone label Jan 16, 2020

IronPan added 3 commits January 15, 2020 22:42

Update kustomization.yaml

15faff1

Update deploy-pipeline-lite.sh

ecca4ab

Update ml-pipeline-viewer-crd-sa.yaml

ecc9b6b

k8s-ci-robot added the lgtm label Jan 16, 2020

update tests

e28cbb7

k8s-ci-robot removed the lgtm label Jan 16, 2020

IronPan added 2 commits January 15, 2020 22:53

update tests

1d0eb3b

update tests

2c35dbd

k8s-ci-robot added the lgtm label Jan 16, 2020

k8s-ci-robot added the approved label Jan 16, 2020

k8s-ci-robot merged commit a0a39a5 into kubeflow:master Jan 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install application CRD and add pipeline application CR to pipeline standalone #2585

Install application CRD and add pipeline application CR to pipeline standalone #2585

IronPan commented Nov 9, 2019 •

edited by jlewi

Loading

IronPan commented Nov 9, 2019

IronPan commented Nov 9, 2019

Bobgy left a comment

Bobgy commented Nov 9, 2019

IronPan commented Nov 11, 2019

Bobgy Nov 12, 2019

Bobgy Nov 12, 2019

IronPan Dec 16, 2019

IronPan Dec 16, 2019

Bobgy Dec 24, 2019

rmgogogo Dec 19, 2019

Bobgy Dec 23, 2019

IronPan Jan 7, 2020

Bobgy left a comment

Bobgy Dec 23, 2019

rmgogogo commented Jan 9, 2020

Bobgy left a comment

Bobgy Jan 16, 2020

Bobgy commented Jan 16, 2020

Bobgy commented Jan 16, 2020

IronPan commented Jan 16, 2020

k8s-ci-robot commented Jan 16, 2020

Install application CRD and add pipeline application CR to pipeline standalone #2585

Install application CRD and add pipeline application CR to pipeline standalone #2585

Conversation

IronPan commented Nov 9, 2019 • edited by jlewi Loading

IronPan commented Nov 9, 2019

IronPan commented Nov 9, 2019

Bobgy left a comment

Choose a reason for hiding this comment

Bobgy commented Nov 9, 2019

IronPan commented Nov 11, 2019

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

rmgogogo commented Jan 9, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented Jan 16, 2020

Bobgy commented Jan 16, 2020

IronPan commented Jan 16, 2020

k8s-ci-robot commented Jan 16, 2020

IronPan commented Nov 9, 2019 •

edited by jlewi

Loading