-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[manifest] Added manifest for deploying on aws using s3 #2633
Conversation
Hi @eterna2. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
containers: | ||
- name: ml-pipeline-ui | ||
env: | ||
- name: AWS_ACCESS_KEY_ID |
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.
@IronPan In future we should probably switch to getting the keys from ConfigMap.
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.
access_key_id and access_key sounds like they belong to a secret. I think this is appropriate.
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.
access_key_id and access_key sounds like they belong to a secret.
AFAIK, they're not the secret itself, more like secret name.
P.S. If the Frontend currently gets those configurations from the ENV, then I guess this PR just reflects that. We should move them to configMap when we have a chance.
key: $(awsSecretKeySecretKey) | ||
- name: ARGO_ARCHIVE_LOGS | ||
value: "true" | ||
- name: ARGO_ARCHIVE_ARTIFACTORY |
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 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.
Thanks for the contribution!
I can see your efforts in this and I believe this could be helpful to many aws users.
My concerns are:
This repo doesn't have test infra to verify these configurations keep working. Therefore, no one can maintain/guarantee these are still up-to-date after future changes.
I suggest
- make a repo of your own for these manifests and pin a pipeline release version as base, so that it doesn't break
- add a readme page here pointing to your repo mentioning it's a variant maintained by you/community, it may not always be up-to-date with latest pipelines, but it is a good reference + any one can contribute to it after they figured out how to patch a later pipelines release.
containers: | ||
- name: ml-pipeline-ui | ||
env: | ||
- name: AWS_ACCESS_KEY_ID |
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.
access_key_id and access_key sounds like they belong to a secret. I think this is appropriate.
}, | ||
{ | ||
"name": "AWS_REGION", | ||
"value": "ap-southeast-1" |
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.
Should this be configurable?
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.
Yeah. Missed this.
Good idea. I will do that and revert this commit and update the readme instead. |
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.
Overall looks good to me, my suggestion is to provide flexibility to support k2iam, EKS IAM for pod feature or other iam solutions
template: | ||
metadata: | ||
annotations: | ||
iam.amazonaws.com/role: $(awsIAMRole) |
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 make the key
configurable? iam.amazonaws.com/role
is for k2iam users. As EKS release feature https://aws.amazon.com/blogs/opensource/introducing-fine-grained-iam-roles-service-accounts/, we have lots of users use eks.amazonaws.com/role-arn
as well.
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.
kustomize don't allow templating for keys yet - i.e. I cannot do $(key): $(value)
for annotation.
a possible workaround is to use commonAnnotations
(actually this is what I used for my deployment, cuz I was lazy. I gave all kfp deployments the same iam role)
This reverts commit 6a9c498.
Hey all, Created a new repo: https://github.com/e2fyi/kubeflow-aws to hold the manifest. Streamlined the manifest too.
I also added a short write-up on the various components of kubeflow pipelines in my repo. Probably useful for pple to have some sense where and what to look at if they want to do something. Maybe I shld put this somewhere too - probably under developer notes?
For this PR:
|
manifests/kustomize/README.md
Outdated
provides a community-maintained manifest for deploying kubeflow pipelines on AWS | ||
(with S3 as artifact store instead of minio). | ||
|
||
TL;DR |
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.
Shall we also move these instructions into the new repo?
I think that makes it more self contained, and instruction + manifests are versioned together makes more sense
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.
We should probably still have at least a small section about AWS and a pointer to the other repo. @IronPan WDYT?
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.
Agree, I was thinking the section only introduces the repo.
Ok. I have removed the unnecessary deployment instructions in the readme. |
/lgtm Thanks! this looks good |
[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:
Approvers can indicate their approval by writing |
/ok-to-test |
There are a number of requests both in the issues (#2627 #1610 #1131) as well as in the slack channel on how to configure kubeflow pipeline on AWS (namely interacting with S3 buckets, particularly for pipeline-ui).
This PR adds 2 kustomize overlays:
The more common configurations (e.g. bucket, folder) are also exposed inside
params.env
.archiveLogs
for argo is also set totrue
so that the logs are persisted and can be retrieved by the ui.A README is also provided in the root dir of the overlays.
This change is