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

moving kfserving to 0.2.2 #631

Merged
merged 6 commits into from
Dec 4, 2019
Merged

moving kfserving to 0.2.2 #631

merged 6 commits into from
Dec 4, 2019

Conversation

animeshsingh
Copy link
Contributor

@animeshsingh animeshsingh commented Nov 26, 2019 •
edited by jlewi
Loading

Moves kfserving to 0.2.2

This change is Reviewable

@animeshsingh
Copy link
Contributor Author

/assign @johnugeorge

cc @jlewi @ellis-bigelow

@animeshsingh animeshsingh mentioned this pull request Nov 27, 2019
Merged
@yuzisun
Copy link
Member

yuzisun commented Nov 27, 2019

I think there are other changes like logger in the crd not just version bump, we need a better way to sync manifests in kfserving to here.

@animeshsingh
Copy link
Contributor Author

animeshsingh commented Nov 27, 2019 •
edited
Loading

@yuzisun is the CRD pulled from kfserving to here? Or manually crafted?
https://github.com/kubeflow/kfserving/blob/master/config/default/crds/serving_v1alpha2_inferenceservice.yaml

@yuzisun
Copy link
Member

yuzisun commented Dec 2, 2019

@yuzisun is the CRD pulled from kfserving to here? Or manually crafted?
https://github.com/kubeflow/kfserving/blob/master/config/default/crds/serving_v1alpha2_inferenceservice.yaml

You can not pull from config there because it does some patch later on with kustomize. Unfortunately for now you need to manually pull the final crd out from the 0.2.2 release yaml(https://github.com/kubeflow/kfserving/blob/master/install/0.2.2/kfserving.yaml), we need a script to automate this process, manual update here is a bit error prone.

@yuzisun
Copy link
Member

yuzisun commented Dec 2, 2019

@animeshsingh most importantly we need to add the namespace selector environment variable on kfserving controller statefulset manifest @hougangliu introduced :)

@animeshsingh
Copy link
Contributor Author

Ok - seems the tests are looking good now + accounted for namespace label selector change.

@yuzisun
Copy link
Member

yuzisun commented Dec 2, 2019

awesome! looks good now!
/lgtm

@animeshsingh
Copy link
Contributor Author

@krishnadurai or @kkasravi this one will need attention

@johnugeorge
Copy link
Member

/lgtm

krishnadurai
krishnadurai reviewed Dec 3, 2019
View reviewed changes
Copy link
Contributor

@krishnadurai krishnadurai left a comment

Choose a reason for hiding this comment

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

/lgtm

Just a minor comment.

kfserving/kfserving-install/base/statefulset.yaml
@@ -39,7 +39,9 @@ spec:
fieldPath: metadata.namespace
- name: SECRET_NAME
value: kfserving-webhook-server-secret
image: $(registry)/kfserving-controller:0.2.0
- name: ENABLE_WEBHOOK_NAMESPACE_SELECTOR
value: enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this value to be configured by KfDef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - enables kfserving deployment in kubeflow to be independent of standalone kfserving behaviour

Copy link
Member

Choose a reason for hiding this comment

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

with kubeflow installation I think this should be always be on otherwise user is taking a risk. Once we migrate to object selector with kuberentes 1.15+ then this env can be retired.

@animeshsingh animeshsingh mentioned this pull request Dec 4, 2019
Merged
@animeshsingh
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh

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

@k8s-ci-robot k8s-ci-robot merged commit 5737682 into kubeflow:master Dec 4, 2019
@animeshsingh animeshsingh mentioned this pull request Dec 5, 2019
Closed
k8s-ci-robot pushed a commit that referenced this pull request Dec 6, 2019
@lluunn @k8s-ci-robot
Automated cherry pick of #631: moving kfserving to 0.2.2 Cherry pick of
5c308ef 
#631 on v0.7-branch. #631: moving kfserving to 0.2.2 (#650)

* moving kfserving to 0.2.2

* move tests to 0.2.2

* add logger

* changing stateful set, cluster-role.yaml and crd

* changing crd.yaml

* updating tests

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

@krishnadurai krishnadurai krishnadurai left review comments

@yuzisun yuzisun yuzisun left review comments

@IronPan IronPan Awaiting requested review from IronPan

Assignees

@johnugeorge johnugeorge

@krishnadurai krishnadurai

@yuzisun yuzisun

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@animeshsingh @yuzisun @johnugeorge @k8s-ci-robot @krishnadurai @googlebot