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.

Sign up for GitHub

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

Jump to bottom

[IBM] Add kfp-tekton deployment and update IBM stacks #1621

Merged
merged 2 commits into from
Nov 10, 2020
Merged

[IBM] Add kfp-tekton deployment and update IBM stacks #1621

merged 2 commits into from
Nov 10, 2020

Conversation

Tomcli
Copy link
Member

@Tomcli Tomcli commented Nov 9, 2020

Which issue is resolved by this Pull Request:
Resolves #

Description of your changes:

  1. Add new installs for kfp-tekton and kfp-tekton multi-user
  2. Update IBM Stacks to use kfp-tekton installs
  3. Update all IBM KfDef to use the "custom" stack style similar to the openshift kfdef. Removing the ibm_custom_stack.yaml since it's incorporated as part of the latest IBM kfdef.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@google-cla google-cla bot added the cla: yes label Nov 9, 2020
@Tomcli
Copy link
Member Author

Tomcli commented Nov 9, 2020

/cc @moficodes

@k8s-ci-robot
Copy link
Contributor

@Tomcli: GitHub didn't allow me to request PR reviews from the following users: moficodes.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @moficodes

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.

@Tomcli
Copy link
Member Author

Tomcli commented Nov 9, 2020

/cc @animeshsingh

@animeshsingh
Copy link
Contributor

/lgtm

cc @pvaneck for quick review

pvaneck
pvaneck reviewed Nov 10, 2020
View reviewed changes
Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

Hey, looks good. Just a quick question. Do the YAML files in this PR reflect the resources listed here: https://github.com/kubeflow/kfp-tekton/blob/master/install/v0.4.0/kfp-tekton.yaml ? Noticed that some roles like ml-pipeline-persistenceagent-role were kind Role in that file but ClusterRole here, so I'm curious what prompts these changes in this manifests installation context.

pipeline/installs/tekton/OWNERS Outdated Show resolved Hide resolved
pipeline/minio/installs/ibm/deployment-patch.yaml
@@ -2,6 +2,8 @@ apiVersion: apps/v1
kind: Deployment
metadata:
name: minio
annotations:
sidecar.istio.io/inject: "false"
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious. Were you were having sidecar issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

For multi-user, the Kubeflow namespace needs to have sidecar injection. However, Minio is not suppose to have sidecar and this was done as part of the single ibm stack deployment. However, with the new independent application approach, I need to remove all the dependencies between each application. I put the annotations to our Minio overlays so that it can be deployed after the sidecar injection labels being patched in the previous applications.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 10, 2020
animeshsingh
animeshsingh approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@animeshsingh animeshsingh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot merged commit 7f563d2 into kubeflow:master Nov 10, 2020
moficodes
moficodes approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@moficodes moficodes left a comment

Choose a reason for hiding this comment

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

lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh, moficodes, Tomcli

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

This was referenced Nov 10, 2020
Closed
Merged
k8s-ci-robot pushed a commit that referenced this pull request Nov 10, 2020
@Tomcli @animeshsingh
Automated cherry pick of #1621 #1627 [IBM] Add kfp-tekton deployment …
da3fb83 
…and update IBM stacks (#1628)

* add kfp-tekton deployment and update IBM stacks

* update owner file

* pin ibm v1.2 manifests

* Apply suggestions from code review

Co-authored-by: Animesh Singh <singhan@us.ibm.com>

Co-authored-by: Animesh Singh <singhan@us.ibm.com>
animeshsingh added a commit to animeshsingh/manifests that referenced this pull request Nov 11, 2020
@animeshsingh
Add Paul as a reviewer
ec58f7d 
Will be responsible for maintaining manifests for KFServing, OpenShift, IKS etc.

PRs merged: 
kubeflow#1515
kubeflow#1575
kubeflow#1580
kubeflow#1617

Reviews:
kubeflow#1582
kubeflow#1621
kubeflow#1625
kubeflow#1627
kubeflow#1628
kubeflow/kfctl#419
@animeshsingh animeshsingh mentioned this pull request Nov 11, 2020
Merged
animeshsingh added a commit to animeshsingh/internal-acls that referenced this pull request Nov 11, 2020
@animeshsingh
add @moficodes to Kubeflow org
de3b9e3 
PRs:
Manifest (kubeflow/manifests#1603)
Website (kubeflow/website#1752)

Reviews:
Manifest(kubeflow/manifests#1621, kubeflow/manifests#1594,  kubeflow/manifests#1582, kubeflow/manifests#1567)
kfctl(kubeflow/kfctl#419)
Website(kubeflow/website#2327)

Issue Triage:
kfctl(kubeflow/kfctl#428, kubeflow/kfctl#424, kubeflow/kfctl#418)
@animeshsingh animeshsingh mentioned this pull request Nov 11, 2020
Merged
k8s-ci-robot pushed a commit to kubeflow/internal-acls that referenced this pull request Nov 11, 2020
@animeshsingh
add @moficodes to Kubeflow org (#399)
8793ddf 
PRs:
Manifest (kubeflow/manifests#1603)
Website (kubeflow/website#1752)

Reviews:
Manifest(kubeflow/manifests#1621, kubeflow/manifests#1594,  kubeflow/manifests#1582, kubeflow/manifests#1567)
kfctl(kubeflow/kfctl#419)
Website(kubeflow/website#2327)

Issue Triage:
kfctl(kubeflow/kfctl#428, kubeflow/kfctl#424, kubeflow/kfctl#418)
k8s-ci-robot pushed a commit that referenced this pull request Nov 11, 2020
@animeshsingh
Add Paul as a reviewer (#1632)
fcd557a 
* Add Paul as a reviewer

Will be responsible for maintaining manifests for KFServing, OpenShift, IKS etc.

PRs merged: 
#1515
#1575
#1580
#1617

Reviews:
#1582
#1621
#1625
#1627
#1628
kubeflow/kfctl#419

* Update OWNERS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvaneck pvaneck pvaneck left review comments

@animeshsingh animeshsingh animeshsingh approved these changes

@moficodes moficodes moficodes approved these changes

@Bobgy Bobgy Awaiting requested review from Bobgy

@krishnadurai krishnadurai Awaiting requested review from krishnadurai

Assignees

@animeshsingh animeshsingh

Labels
approved cla: yes lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Tomcli @k8s-ci-robot @animeshsingh @pvaneck @moficodes