-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add third party license + source code to argo and minio images to comply with their license #2201
Conversation
Notes
|
@IronPan is OOO. @james-jwu @dushyanthsc can you help me review this? |
TODO:
|
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. 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: |
/lgtm |
I totally agree, let me try to refactor like this. |
/lgtm cancel |
/hold |
Plan
|
…uild each image, also added README for instructions
@@ -9,8 +9,6 @@ bases: | |||
- metadata | |||
|
|||
images: | |||
- name: argoproj/workflow-controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no usage of argoproj/workflow-controller. So I simply removed this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty certain that the workflow controller is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we are using gcr.io/ml-pipeline/workflow-controller, but not argoproj/workflow-controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty certain it's used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you let me know where it is? I couldn't find it when I searched for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's already referencing the gcr image:
image: gcr.io/ml-pipeline/workflow-controller:v2.3.0 |
argoexec
image is already there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I found too. Do you have any further concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/hold cancel |
Note that I've manually released these '-license-compliance' images on 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. |
Asking to review third_party/README.md, Dockerfiles and manifests. I am least confident with these. |
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 |
We can add some suffixes. E.g.
etc |
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 |
@@ -141,10 +141,10 @@ steps: | |||
waitFor: ['PullMetadataEnvoy'] | |||
|
|||
- name: 'gcr.io/cloud-builders/docker' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/lgtm /cc @neuromage Do you have any remaining concerns about this PR? |
/lgtm |
[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:
Approvers can indicate their approval by writing |
@Bobgy please also see my comment on the naming scheme. |
Hi @Ark-kun and @neuromage, thanks for the review and approval. I think there are some common misunderstandings I didn't clarify well.
Also, summarize about how these images were built:
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:
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.
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.
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>
Prepare required OSS licenses in the image.
/assign @IronPan
/cc @dushyanthsc
/cc @james-jwu
This change is