Skip to content

Fix for Seldon custom namespace installs of kubeflow#1375

Merged
k8s-ci-robot merged 6 commits into
kubeflow:masterfrom
ukclivecox:seldon_1.2.1
Jul 13, 2020
Merged

Fix for Seldon custom namespace installs of kubeflow#1375
k8s-ci-robot merged 6 commits into
kubeflow:masterfrom
ukclivecox:seldon_1.2.1

Conversation

@ukclivecox

@ukclivecox ukclivecox commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

Which issue is resolved by this Pull Request:
Resolves #1022

Description of your changes:

  • Add kustomizations to allow namespace to be used for
    • Certificates
    • Webhooks
    • Kubeflow gateway setting
  • Update to Seldon 1.2.1

Also need to change validation tests as CRDs can have status fields and this was being rejected.

Checklist:

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

@kubeflow-bot

kubeflow-bot commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

This change is Reviewable

@ukclivecox ukclivecox changed the title Seldon 1.2.1 Fix for Seldon custom namespace installs of kubeflow Jul 10, 2020
@ukclivecox

ukclivecox commented Jul 10, 2020

Copy link
Copy Markdown
Contributor Author

@vpavlin
@yuzisun

@ukclivecox

ukclivecox commented Jul 10, 2020

Copy link
Copy Markdown
Contributor Author

@animeshsingh this will need your approval as well as it affects the ibm stack that imports seldon
@jlewi I think we need this for 1.1 release. A similar issue will need fixing for kfserving
@ryandawsonuk can you review from seldon side?

@ryandawsonuk

ryandawsonuk commented Jul 10, 2020

Copy link
Copy Markdown

Looks good from Seldon perspective

@animeshsingh

animeshsingh commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

/lgtm

adrian555
adrian555 reviewed Jul 10, 2020
View reviewed changes
Comment thread seldon/Makefile
mkdir -p seldon-core-operator/base
cd seldon-core-operator/base && helm template -f ../../values.yaml seldon-core seldon-core-operator --repo https://storage.googleapis.com/seldon-charts --namespace kubeflow --version 1.1.0 > resources.yaml
cd seldon-core-operator/base && helm template -f ../../values.yaml seldon-core seldon-core-operator --repo https://storage.googleapis.com/seldon-charts --namespace kubeflow --version 1.2.1 > resources.yaml
sed -i 's#cert-manager.io/inject-ca-from:.*#cert-manager.io/inject-ca-from: $$(CERTIFICATE_NAMESPACE)/$$(CERTIFICATE_NAME)#g' seldon-core-operator/base/resources.yaml

@adrian555 adrian555 Jul 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this supposed to run on MacOS? If yes, please fix the sed -i command.

@ukclivecox ukclivecox Jul 11, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The present OWNERS run linux. Could we punt this to a future small PR fix? @adrian555

@adrian555 adrian555 Jul 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

k

ryandawsonuk
ryandawsonuk approved these changes Jul 13, 2020
View reviewed changes

@ryandawsonuk ryandawsonuk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

/lgtm
/approve

@ukclivecox

ukclivecox commented Jul 13, 2020

Copy link
Copy Markdown
Contributor Author

/assign @animeshsingh

@vpavlin

vpavlin commented Jul 13, 2020

Copy link
Copy Markdown
Member

I briefly tested it and it looks good!

Thanks a lot @cliveseldon

/lgtm

@animeshsingh

animeshsingh commented Jul 13, 2020

Copy link
Copy Markdown
Contributor

/approve

@k8s-ci-robot

k8s-ci-robot commented Jul 13, 2020

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh, ryandawsonuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@k8s-ci-robot k8s-ci-robot merged commit 13062e9 into kubeflow:master Jul 13, 2020
@jlewi jlewi mentioned this pull request Jul 14, 2020
Closed
k8s-ci-robot pushed a commit that referenced this pull request Jul 20, 2020
@ukclivecox
Seldon 1.2.1 cherrypick to v1.1 branch (#1395)
f65a938 
* Fix for Seldon custom namespace installs of kubeflow (#1375)

* Allow namespace install and update to 1.2.1

* remove duplicate vars

* update for 1.2.1 and automation

* update version

* Update tests and change validate to allows CRDs with status

* Revert CRD status exception and remove status from Seldon CRD (#1389)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@krishnadurai krishnadurai Awaiting requested review from krishnadurai

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan

2 more reviewers

@adrian555 adrian555 adrian555 left review comments

@ryandawsonuk ryandawsonuk ryandawsonuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@animeshsingh animeshsingh

@vpavlin vpavlin

@ryandawsonuk ryandawsonuk

Labels

approved cla: yes lgtm size/XL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Seldon fails if KF is deployed to a custom namespace

8 participants

@ukclivecox @kubeflow-bot @ryandawsonuk @animeshsingh @vpavlin @k8s-ci-robot @adrian555 @googlebot