Skip to content
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

refactor(deployment): refactor argo manifests to be overlay on top of upstream #5273

Merged
merged 9 commits into from
Mar 11, 2021

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Mar 10, 2021

Description of your changes:

Problem: KFP maintains its own fork of argo manifests
Cons:

  • it's not obvious what's changed from upstream, so it's hard to maintain & hard for people to bring their own argo installation
  • it's hard to upgrade argo manifests when there are argo manifest breaking changes

Proposed Solution: pull argo manifests as an upstream folder and build KFP specific argo overlays.

Challenges:

  • KFP standalone doc asks users to install via kubectl apply -k, but upstream argo manifests are not compatible with the outdated kustomize version in kubectl.

Changes:

  • pull argo manifests from upstream repo directly
  • build overlay on top of the upstream manifests
  • avoided all env usages in common manifests, so that they are compatible with any kustomize version
  • switch to PNS executor. Fixes Support for non-docker based deployments #1654 cc @capri-xiyue
  • add a sample argo installation that should work with KFP multi-user mode

Checklist:

@google-oss-robot
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 10, 2021

/assign @yanniszark @thesuperzapper

I think you will be interested in the PR.

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 10, 2021

/cc @zijianjoy

Copy link
Contributor Author

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems hard to review, so I think I'll have to just make a call to merge.
Before I do that, I'd like some feedback if this seems like the right direction to go, based on PR description.

/cc @chensun @yanniszark

Comment on lines +14 to +28
# In artifactRepository.s3.endpoint, $(kfp-namespace) is needed, because in multi-user mode, pipelines may run in other namespaces.
artifactRepository: |
archiveLogs: true
s3:
endpoint: "minio-service.$(kfp-namespace):9000"
bucket: "$(kfp-artifact-bucket-name)"
keyFormat: "artifacts/{{workflow.name}}/{{pod.name}}"
# insecure will disable TLS. Primarily used for minio installs not configured with TLS
insecure: true
accessKeySecret:
name: mlpipeline-minio-artifact
key: accesskey
secretKeySecret:
name: mlpipeline-minio-artifact
key: secretkey
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to move this patch to minio manifests, but it was more challenging than I thought.
I recorded why that's hard in #5276 and discussed some potential solutions.

@chensun
Copy link
Member

chensun commented Mar 11, 2021

/lgtm

@google-oss-robot google-oss-robot merged commit 09a7236 into kubeflow:master Mar 11, 2021
@Bobgy Bobgy deleted the argo-manifests-refactor branch March 11, 2021 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for non-docker based deployments
5 participants