Add REST API using grpc gateway #142

mayankjuneja · 2018-08-06T10:54:55Z

We have been using Katib internally and one of the changes we have done is to expose core service as REST APIs (using grpc-gateway). I think this might be useful for others as well (issues #117, #118).

Side note - You might notice a lot of changes to vendor dir (these are mostly deletions). Doing a dep ensure cleaned up some of the unused packages. If there are other best practices around this, please let me know. I can also create a separate PR to clean up the unused packages.

This change is

vinaykakade · 2018-08-06T11:08:48Z

@YujiOshima - is there any advantage of checking-in the vendor folder with Katib? Checking in gopkg.lock and gopkg.toml should be sufficient. The PRs will be easier to look at that way, and any needed vendor packages can be downloaded locally based on lock/toml and without needing the vendor folder checked-in.

YujiOshima · 2018-08-06T15:47:16Z

@mayankjuneja Thank you for the great contribution! I will take a look. Could you add some documents for REST?

@vinaykakade There is no exactry answer. https://github.com/golang/dep/blob/master/docs/FAQ.md#should-i-commit-my-vendor-directory
We can protect it from deletion of depending repository by including vendor folders.
the more Katib mature, the less vendored folders change.

YujiOshima · 2018-08-06T15:47:35Z

/ok-to-test

YujiOshima · 2018-08-06T15:47:43Z

/assign YujiOshima

mayankjuneja · 2018-08-06T15:57:13Z

@YujiOshima sure, I will add some documentation/comments.

mayankjuneja · 2018-08-08T08:28:03Z

pkg/api/build.sh

@@ -1 +1,3 @@
 docker run -it --rm -v $PWD:$(pwd) -w $(pwd) znly/protoc --python_out=plugins=grpc:./python --go_out=plugins=grpc:. -I. api.proto
+docker run -it --rm -v $PWD:$(pwd) -w $(pwd) znly/protoc --grpc-gateway_out=logtostderr=true:. -I. api.proto


building the grpc gateway and swagger stub files

mayankjuneja · 2018-08-08T08:29:23Z

scripts/build.sh

@@ -28,6 +28,9 @@ cd ${SCRIPT_ROOT}
 echo "Building core image..."
 docker build -t ${PREFIX}/vizier-core -f ${CMD_PREFIX}/manager/Dockerfile .

+echo "Building REST API for core image..."
+docker build -t ${PREFIX}/vizier-core-rest -f ${CMD_PREFIX}/manager_rest/Dockerfile .


build the image for manager REST API. it will be a separate service running in kubernetes.

mayankjuneja · 2018-08-08T08:35:18Z

@YujiOshima I added some comments in the code and in this PR as well. Let me know if I should add more details.

pkg/api/api.proto

YujiOshima · 2018-08-09T04:58:09Z

@mayankjuneja

Please add build test for core-rest in CI.

katib/test/scripts/run-tests.sh

Line 45 in ecbc5dd

sed -i -e "s@image: katib\/vizier-core@image: ${REGISTRY}\/${REPO_NAME}\/vizier-core:${VERSION}@" manifests/vizier/core/deployment.yaml
Could you add doc to doc dir for how to use rest API with curl or other client?

Thank you!

jlewi · 2018-08-20T01:18:20Z

Any update on this? Would be great to get it committed?

mayankjuneja · 2018-09-20T08:31:20Z

@YujiOshima I did another pass and made the following changes:

added more documentation (in docs folder)
merged changes from master
updated the build, deploy scripts.

Can you review these changes? Thanks!

YujiOshima · 2018-09-21T03:45:41Z

manifests/vizier/core-rest/rbac.yaml

@@ -0,0 +1,37 @@
+---


Since core-rest does not need to manage k8s resources, the rbac looks unnecessary.

good point, thanks for catching this!

YujiOshima · 2018-09-21T03:48:27Z

docs/rest-api.md

+For each RPC service, we define an equivalent HTTP REST API method using [grpc-gateway](https://github.com/grpc-ecosystem/grpc-gateway). The mapping between service and REST API method can be found in this [file](https://github.com/kubeflow/katib/blob/master/pkg/api/api.proto). The mapping includes the URL path, query parameters and request body. You can read more details on the supported methods and binding options at this [link](https://cloud.google.com/service-infrastructure/docs/service-management/reference/rpc/google.api#http)
+
+## Examples
+Assuming `{HOST}` as the ingress host for vizier-core-rest, 


Please add a way of using portforward.

kubectl v1.10~
$ kubectl -n katib port-forward svc/vizier-core-rest 8080:80

kubectl ~v1.9
& kubectl -n katib port-forward $(kubectl -n katib get pod -o=name | grep vizier-core-rest | sed -e "s@pods\/@@") 8080:80

I will add this.

YujiOshima · 2018-09-21T03:53:02Z

docs/rest-api.md

+Request:
+
+```shell
+curl -X POST {HOST}/api/Manager/CreateStudy


Write explicitly to use above json file.
curl -X POST {HOST}/api/Manager/CreateStudy -d @study_config.json

sorry, missed the argument here.

YujiOshima · 2018-09-21T03:57:23Z

@mayankjuneja Thank for the great contribution!
Generally looks good. I checked it could work!
I commented a few points.
We can merge after fixing them.

mayankjuneja · 2018-09-21T04:15:01Z

@YujiOshima thanks for the review! I added some changes based on your comments. Let me know if it looks good.

YujiOshima · 2018-09-21T04:43:05Z

@mayankjuneja Thanks!
/lgtm
/approve

k8s-ci-robot · 2018-09-21T04:43:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: YujiOshima

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:

~~OWNERS~~ [YujiOshima]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mayankjuneja · 2018-09-21T05:00:51Z

/retest

YujiOshima · 2018-09-21T05:03:11Z

@mayankjuneja I think you need to remove serviceAccountName: vizier-core-rest from core-rest deployment.

mayankjuneja · 2018-09-21T05:54:01Z

@YujiOshima The tests passed now, can you please reapprove it?

YujiOshima · 2018-09-21T06:26:15Z

/lgtm

YujiOshima · 2018-09-21T06:28:48Z

🎉🎉🎉🎉🎉🎉🎉

Mayank Juneja added 10 commits August 6, 2018 15:07

dep ensure

6ce4191

add grpc-gateway via dep

65683a4

update protobuf via dep ensure

3c212c2

update compiled go code, add reverse proxy

6ea6cf7

add REST entrypoint for manager

d9fdfd5

update API build script

b4b54df

use build script to generate code

72a1108

remove binary file

e61a24e

update build, deploy scripts for REST API

e427ad4

change name

230f66c

k8s-ci-robot added the size/XXL label Aug 6, 2018

k8s-ci-robot requested review from ddysher and libbyandhelen August 6, 2018 10:55

k8s-ci-robot added the needs-ok-to-test label Aug 6, 2018

k8s-ci-robot removed the needs-ok-to-test label Aug 6, 2018

k8s-ci-robot assigned YujiOshima Aug 6, 2018

Mayank Juneja added 2 commits August 6, 2018 21:49

add manifests for core-rest

5a98cd7

remove deploy

6e3481b

mayankjuneja commented Aug 8, 2018

View reviewed changes

add comments

ecbc5dd

YujiOshima reviewed Aug 9, 2018

View reviewed changes

pkg/api/api.proto Outdated Show resolved Hide resolved

Mayank Juneja added 8 commits September 19, 2018 15:12

update build scripts and tests

1ea4350

copy vendor for tests

138f0f6

uncomment deploy

75e1d8e

update image name

7751637

ignore vizier-core-rest for port forwarding

44be303

update build script

63bafb2

update manifests

285d9f2

Add docs for REST API

ee3f03e

YujiOshima reviewed Sep 21, 2018

View reviewed changes

core review changes

f564aa8

k8s-ci-robot added the lgtm label Sep 21, 2018

k8s-ci-robot added the approved label Sep 21, 2018

remove service account

c7ee0cb

k8s-ci-robot removed the lgtm label Sep 21, 2018

k8s-ci-robot added the lgtm label Sep 21, 2018

k8s-ci-robot merged commit 81f2b74 into kubeflow:master Sep 21, 2018

mayankjuneja deleted the grpc_gateway branch September 21, 2018 06:29

YujiOshima mentioned this pull request Sep 25, 2018

REST api #117

Closed

andreyvelich mentioned this pull request Jul 24, 2020

GRPC: Rename Manager to DBManager service #1279

Merged

andreyvelich mentioned this pull request Mar 20, 2021

Fix gRPC manager build script #1492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add REST API using grpc gateway #142

Add REST API using grpc gateway #142

mayankjuneja commented Aug 6, 2018 •

edited

Loading

vinaykakade commented Aug 6, 2018

YujiOshima commented Aug 6, 2018

YujiOshima commented Aug 6, 2018

YujiOshima commented Aug 6, 2018

mayankjuneja commented Aug 6, 2018

mayankjuneja Aug 8, 2018

mayankjuneja Aug 8, 2018

mayankjuneja commented Aug 8, 2018

YujiOshima commented Aug 9, 2018

jlewi commented Aug 20, 2018

mayankjuneja commented Sep 20, 2018

YujiOshima Sep 21, 2018

mayankjuneja Sep 21, 2018

YujiOshima Sep 21, 2018

mayankjuneja Sep 21, 2018

YujiOshima Sep 21, 2018

mayankjuneja Sep 21, 2018

YujiOshima commented Sep 21, 2018

mayankjuneja commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

mayankjuneja commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

mayankjuneja commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

		@@ -1 +1,3 @@
		docker run -it --rm -v $PWD:$(pwd) -w $(pwd) znly/protoc --python_out=plugins=grpc:./python --go_out=plugins=grpc:. -I. api.proto
		docker run -it --rm -v $PWD:$(pwd) -w $(pwd) znly/protoc --grpc-gateway_out=logtostderr=true:. -I. api.proto

Add REST API using grpc gateway #142

Add REST API using grpc gateway #142

Conversation

mayankjuneja commented Aug 6, 2018 • edited Loading

vinaykakade commented Aug 6, 2018

YujiOshima commented Aug 6, 2018

YujiOshima commented Aug 6, 2018

YujiOshima commented Aug 6, 2018

mayankjuneja commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankjuneja commented Aug 8, 2018

YujiOshima commented Aug 9, 2018

jlewi commented Aug 20, 2018

mayankjuneja commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YujiOshima commented Sep 21, 2018

mayankjuneja commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

mayankjuneja commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

mayankjuneja commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

YujiOshima commented Sep 21, 2018

mayankjuneja commented Aug 6, 2018 •

edited

Loading