Add third party license + source code to argo and minio images to comply with their license #2201

Bobgy · 2019-09-23T15:42:31Z

Prepare required OSS licenses in the image.

/assign @IronPan
/cc @dushyanthsc
/cc @james-jwu

This change is

Bobgy · 2019-09-23T15:43:54Z

Notes

I will clean up and include scripts that generated the license files in a separate PR.
I'm not familiar with our release process, does my change looks good?

Bobgy · 2019-09-24T05:46:18Z

@IronPan is OOO. @james-jwu @dushyanthsc can you help me review this?

Bobgy · 2019-09-25T05:37:54Z

TODO:
Add source code of MPL dependencies to the image

argoexec
workflow-controller
minio

Ark-kun · 2019-09-25T21:06:14Z

I think it might not be the best practice to re-release the external images every time someone commits to master. This happens many times per day.
I'm not sure there is a need to re-release the images with every KFP release which happens weekly.

It might be best to have a manual scrupt/cloudbuild file to copy+license a particular tagged image and this script can be run manually any time we want to upgrade to new versions.

We should probably use the same tags as the original images, so that our images just have an extra prefix: argo/argoexec:2.3.0 -> gcr.io/kubeflow-pipelines/argo/argoexec:2.3.0.

third_party/download_source.sh

rmgogogo · 2019-09-25T22:53:10Z

/lgtm

Bobgy · 2019-09-26T01:49:51Z

I think it might not be the best practice to re-release the external images every time someone commits to master. This happens many times per day.
I'm not sure there is a need to re-release the images with every KFP release which happens weekly.

It might be best to have a manual scrupt/cloudbuild file to copy+license a particular tagged image and this script can be run manually any time we want to upgrade to new versions.

We should probably use the same tags as the original images, so that our images just have an extra prefix: argo/argoexec:2.3.0 -> gcr.io/kubeflow-pipelines/argo/argoexec:2.3.0.

I totally agree, let me try to refactor like this.

Bobgy · 2019-09-26T02:18:21Z

/lgtm cancel

Bobgy · 2019-09-26T02:18:46Z

/hold
Until I refactor

Bobgy · 2019-09-26T03:55:02Z

Plan

Build a script that deploys argo and minio images with license + source code
Run the script once against ml-pipeline-staging to build the images and then copy it to ml-pipeline gcr.
Submit a PR that changes our manifests to point to gcr.io/ml-pipeline/minio:some-tag ... to use my newly built images
...
Some time later, when we want to use a different version of argo/minio, we need to release the image once again manually.

…uild each image, also added README for instructions

Bobgy · 2019-09-26T08:37:15Z

manifests/kustomize/base/kustomization.yaml

@@ -9,8 +9,6 @@ bases:
 - metadata

 images:
- name: argoproj/workflow-controller


There's no usage of argoproj/workflow-controller. So I simply removed this one.

I'm pretty certain that the workflow controller is being used.

Yes, we are using gcr.io/ml-pipeline/workflow-controller, but not argoproj/workflow-controller.

I'm pretty certain it's used.

Can you let me know where it is? I couldn't find it when I searched for it.

Looks like it's already referencing the gcr image:

pipelines/manifests/kustomize/base/argo/workflow-controller-deployment.yaml

Line 39 in fbb008f

image: gcr.io/ml-pipeline/workflow-controller:v2.3.0

argoexec image is already there.

Yes, that's what I found too. Do you have any further concerns?

Does that image have the required licenses? Is there some script or cloudbuild entry that can create that image when new Argo version comes out?

gcr.io/ml-pipeline/workflow-controller:v2.3.0 doesn't have license

gcr.io/ml-pipeline/workflow-controller:v2.3.0-license-compliance has license

Is there some script or cloudbuild entry that can create that image when new Argo version comes out?

That's a good point.

I have some dirty scripts for generating the licenses. I made this image from these scripts manually this time.

I think cleaning up these scripts is not a blocker for MKP launch. I will put another PR for it later.

Bobgy · 2019-09-26T08:39:40Z

/hold cancel
I've addressed review comments. Now I'm confident with my approach. More reviews welcomed.

Bobgy · 2019-09-26T08:41:56Z

Note that I've manually released these '-license-compliance' images on ml-pipeline, so tests in this PR are already running with my new images.

Explaining a little about why I named new tags with suffix with the suffix '-license-compliance' because I don't want to replace existing images. There are already people depending on it.

Next time, when we upgrade these 3rd party libraries to a new version, we can just get rid of this suffix.

Bobgy · 2019-09-26T08:44:59Z

Asking to review third_party/README.md, Dockerfiles and manifests. I am least confident with these.

dushyanthsc · 2019-09-26T17:25:58Z

My two cents on the image tagging. I think we should still have some notion of version, because not all images are just vanilla copy of open source image. metadata-envoy for e.g. changes the envoy.yaml and has things like port numbers, etc. which can probably change. So I am completely on-board with not building these images per commit and handle publishing manually, but feel that we should retain some versioning in tagging the image.

Ark-kun · 2019-09-26T19:36:36Z

but feel that we should retain some versioning in tagging the image.

We can add some suffixes. E.g. envoy:1.2.3-kfp

python:3.7.4
python:3.7.4-alpine
python:3.7.4-slim

etc

Bobgy · 2019-09-26T23:46:08Z

I agree with having suffix and keeping original tag name to easily let people know it is customized and also which underlying image it was built from.

For envoy, since we are building sth on top of it. Sounds like we can go with our own versioning too if you prefer. Images don't need all follow the same rule

.release.cloudbuild.yaml

Ark-kun · 2019-09-27T02:35:50Z

.release.cloudbuild.yaml

@@ -141,10 +141,10 @@ steps:
  waitFor: ['PullMetadataEnvoy']

 - name: 'gcr.io/cloud-builders/docker'


Let's move all these entries to a separate cloudbuild.yaml file in the /third-party directory so that it's not triggered with every release.

I don't quite understand why we have these entries. They are copying already built images from gcr.io/ml-pipeline/... to gcr.io/ml-pipeline/google/pipelines/...:$TAG_NAME (released tag name).

But I couldn't find any usage of gcr.io/ml-pipeline/google/pipelines, our lite deployment only references gcr.io/ml-pipeline/image-name.

Can you explain what these are for? I didn't understand so I merely changed the tags to my new tags.

I also checked https://github.com/kubeflow/manifests, they are also not using these. @IronPan do you know what these are for? Just meant as taking a snapshot of the release artifacts?

@Bobgy no need to add license-compliance part in the tag here. As long as we're building these default ones with the source and license included, we're good.

@neuromage My reasoning of the extra license-compliance part: #2201 (comment)

It's for purpose of safe gradual release, because images like gcr.io/ml-pipeline/argoexec:v2.3.0 are already in use today. If I push a new image to the same tag, everyone will switch to that new image immediately.

Technically, there's no difference between previous and new images. New images also just passed our presubmit tests. So risk pushing new images under the same tag is small too. I think we can also choose to do that.

WDYT? I'd like to double check if you want me to push new images under the same tag? or would you want to suggest a simpler naming?

I think if we're distributing the images, we should just version them (since we technically rebuild these every release). Ideally, we would name them something like argoexec-v2.3.0-kfp:0.1.30 which can indicate that this is based of argo v2.3.0, but released under kfp with tag 0.1.30. I don't want to block this PR, but can we consider using this convention? This can be done in a future PR.

/cc @IronPan

argoexec-v2.3.0-kfp:0.1.30
Should we do that even if we only release a modified version of the image once per Argo release (not once per KFP release)?

Using different tags for the same image might be a bit confusing.

third_party/argo/release.sh

Ark-kun · 2019-09-30T21:36:28Z

/lgtm
Let's make another PR where we extract the CloudBuild entries into separate script or scripts which are only run when the new upstream image is released.

/cc @neuromage Do you have any remaining concerns about this PR?

neuromage · 2019-10-01T03:57:00Z

/lgtm
/approve

k8s-ci-robot · 2019-10-01T03:57:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neuromage

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

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

neuromage · 2019-10-01T03:57:44Z

@Bobgy please also see my comment on the naming scheme.

Bobgy · 2019-10-01T20:29:39Z

Hi @Ark-kun and @neuromage, thanks for the review and approval.

I think there are some common misunderstandings I didn't clarify well.

The cloud build entries here pull images from gcr.io/ml-pipeline/xxx-image and retag it to gcr.io/ml-pipeline/google/xxx-image:TAG_NAME. They don't rebuild 3rd party images per release.
Image building for 3rd party images happen in scripts like this. I ran this script manually a few days ago to release images like gcr.io/ml-pipeline/argoexec:v2.3.0-license-compliance. It's an one time effort until we need to use a new version of an image.

Also, summarize about how these images were built:

Collect dependencies + transitive dependencies in these images. (Each repo has a different way, needs to be done manually.)
Run a script to crawl github repos of golang imports (Not all imports can be figured out by my script, needs manual help for <2% of libraries)
Run a script to crawl github license info of these libraries. (Not all repos have github recognizable license, needs manual help for <2% of libraries)
Crawl full text license files for all dependencies + source code for MPL dependencies.
Build the above into an image and push it to gcr.io/ml-pipeline/xxx-image

Because everything before step 4 cannot be automated completely. I commited concatenated license file and a list of MPL dependencies + their source code download url into KFP repo to build 3rd party images just for the version we use.

Action item left: (I will work on after vacation) tidy up and document my scripts that did step 1~4, these steps need to be done each time we upgrade image version.

So... to answer some of the concerns:

Let's make another PR where we extract the CloudBuild entries into separate script.

I don't think these entries need to be extracted, because they don't rebuild images + they were like this before this change too. Although I don't fully understand why we need them, because manifests never reference them. Please help me understand more historic reasons for these entries if I missed anything here.

scripts which are only run when the new upstream image is released.

I've committed scripts I used to build current 3rd party images into the repo. [Mentioned above as Action Item] I need to include scripts that can be used when we upgrade libraries.

I think if we're distributing the images, we should just version them (since we technically rebuild these every release). Ideally, we would name them something like argoexec-v2.3.0-kfp:0.1.30 which can indicate that this is based of argo v2.3.0, but released under kfp with tag 0.1.30. I don't want to block this PR, but can we consider using this convention? This can be done in a future PR.

As explained above, we don't rebuild these images every release. We only need to build one image every time we use a new version of a 3rd party image.

Added a specific version for protobuf in requirements.txt Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

Add concatenated third party license to argo and minio images

8519855

k8s-ci-robot assigned IronPan Sep 23, 2019

k8s-ci-robot requested review from dushyanthsc and james-jwu September 23, 2019 15:42

k8s-ci-robot added the size/XXL label Sep 23, 2019

Add MPL dependencies source code in argoexec docker image

bcf5e44

Bobgy added 2 commits September 25, 2019 15:37

Include source code of MPL dependencies of minio in its image

95ae91d

Add source code for argo dependencies with MPL in argo images

52d8180

Bobgy changed the title ~~Add concatenated third party license to argo and minio images~~ Add third party license + source code to argo and minio images to comply with their license Sep 25, 2019

Ark-kun reviewed Sep 25, 2019

View reviewed changes

third_party/download_source.sh Outdated Show resolved Hide resolved

Ark-kun mentioned this pull request Sep 25, 2019

Consider only re-releasing the envoy container image if upgrading to new version #2238

Closed

k8s-ci-robot assigned rmgogogo Sep 25, 2019

k8s-ci-robot added the lgtm label Sep 25, 2019

k8s-ci-robot removed the lgtm label Sep 26, 2019

k8s-ci-robot added the do-not-merge/hold label Sep 26, 2019

Bobgy added 6 commits September 26, 2019 06:48

Updated workflow to release manually, included cloudbuild config to b…

7d7d8cd

…uild each image, also added README for instructions

docker env naming consistency

b4e1e91

Include release scripts and instructions in third_party/README.md

530a7de

Update README.md

ec30211

undo cloudbuild.yaml changes, update README

8503563

Change argo and minio image tags in manifests

4c1d5cc

Remove unneeded code

a82abca

Bobgy commented Sep 26, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold label Sep 26, 2019

Ark-kun reviewed Sep 27, 2019

View reviewed changes

.release.cloudbuild.yaml Show resolved Hide resolved

Ark-kun reviewed Sep 27, 2019

View reviewed changes

numerology reviewed Sep 30, 2019

View reviewed changes

third_party/argo/release.sh Outdated Show resolved Hide resolved

Fix copyright year

f826297

k8s-ci-robot assigned Ark-kun Sep 30, 2019

k8s-ci-robot added the lgtm label Sep 30, 2019

k8s-ci-robot requested a review from IronPan October 1, 2019 03:56

k8s-ci-robot assigned neuromage Oct 1, 2019

k8s-ci-robot added the approved label Oct 1, 2019

k8s-ci-robot merged commit 51f5a5a into kubeflow:master Oct 1, 2019

Bobgy deleted the 3rd_party_licenses branch October 1, 2019 09:57

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Added a specific version for protobuf (kubeflow#2201)

41964fb

Added a specific version for protobuf in requirements.txt Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

		@@ -141,10 +141,10 @@ steps:
		waitFor: ['PullMetadataEnvoy']

		- name: 'gcr.io/cloud-builders/docker'

Add third party license + source code to argo and minio images to comply with their license #2201

Add third party license + source code to argo and minio images to comply with their license #2201

Conversation

Bobgy commented Sep 23, 2019 • edited by jlewi Loading

Bobgy commented Sep 23, 2019

Bobgy commented Sep 24, 2019

Bobgy commented Sep 25, 2019 • edited Loading

Ark-kun commented Sep 25, 2019 • edited Loading

rmgogogo commented Sep 25, 2019

Bobgy commented Sep 26, 2019

Bobgy commented Sep 26, 2019

Bobgy commented Sep 26, 2019

Bobgy commented Sep 26, 2019

Plan

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 Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Bobgy commented Sep 26, 2019

Bobgy commented Sep 26, 2019 • edited Loading

Bobgy commented Sep 26, 2019

dushyanthsc commented Sep 26, 2019

Ark-kun commented Sep 26, 2019

Bobgy commented Sep 26, 2019 • edited Loading

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

Ark-kun commented Sep 30, 2019

neuromage commented Oct 1, 2019

k8s-ci-robot commented Oct 1, 2019

neuromage commented Oct 1, 2019

Bobgy commented Oct 1, 2019 • edited Loading

Bobgy commented Sep 23, 2019 •

edited by jlewi

Loading

Bobgy commented Sep 25, 2019 •

edited

Loading

Ark-kun commented Sep 25, 2019 •

edited

Loading

Bobgy Sep 30, 2019 •

edited

Loading

Bobgy commented Sep 26, 2019 •

edited

Loading

Bobgy commented Sep 26, 2019 •

edited

Loading

Bobgy commented Oct 1, 2019 •

edited

Loading