Replace grpcio with tonic #2112

Jake-Shadle · 2021-05-28T07:39:50Z

/kind feature

What this PR does / Why we need it:
This PR replaces the Rust SDK's use of grpcio with tonic.

Which issue(s) this PR fixes:
Closes #1300

Special notes for your reviewer:
I'll leave inline review comments on the changes themselves, and comment in #1300 on why tonic is preferred.

agones-bot · 2021-05-28T07:57:37Z

Build Failed 😱

Build Id: e23f165b-9168-455e-bcfd-5ef07ce18926

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

sdks/rust/Cargo.toml

Jake-Shadle · 2021-05-28T07:45:13Z

sdks/rust/Cargo.toml

+async-stream = "0.3"
+http = "0.2"
+prost = "0.7"
+thiserror = "1.0"


This replaces error-chain, which has seen declining usage in the rust ecosystem due to its fairly confusing and heavy approach to errors. That being said, the current error usage is extremely minimal, and we could get rid of thiserror as well and just use a custom error, since it's only used to wrap grpc status messages anyways since the sdk protocol doesn't have its own error types at all.

Jake-Shadle · 2021-05-28T07:46:06Z

sdks/rust/Cargo.toml

+[dependencies.tokio]
+version = "1.0"
+default-features = false
+features = ["sync", "time"]


Tonic is built on top of tokio which is (basically) the standard async runtime in the rust ecosystem.

Jake-Shadle · 2021-05-28T07:46:38Z

sdks/rust/Cargo.toml

+[dependencies.tonic]
+version = "0.4"
+default-features = false
+features = [
+    "codegen",
+    "transport",
+    "prost",
+]


Tonic is a rust native GRPC stack built on top of tokio.

Jake-Shadle · 2021-05-28T07:47:55Z

sdks/rust/Cargo.toml

+[build-dependencies.tonic-build]
+version = "0.4"
+default-features = false
+features = [
+    "prost",
+    "transport",
+]


The common practice for tonic is to build the protocol buffers at build time, rather than checking the generated code into source, unlike the previous implementation. I can change this if needed.

sdks/rust/src/sdk.rs

Jake-Shadle · 2021-05-28T08:08:38Z

sdks/rust/src/sdk.rs

-    pub fn ready(&self) -> Result<()> {
-        let req = sdk::Empty::default_instance();
-        let res = self.client.ready(req).map(|_| ())?;
-        Ok(res)
+    #[inline]
+    pub async fn ready(&mut self) -> Result<()> {
+        Ok(self.client.ready(empty()).await.map(|_| ())?)
    }


Just as with the Alpha client, tonic is async only, so just removed all of the sync methods.

sdks/rust/src/sdk.rs

agones-bot · 2021-05-28T09:43:34Z

Build Failed 😱

Build Id: 4deaee5a-ab75-4a45-a3f6-267b1a6fa3d8

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2021-05-28T10:46:04Z

Build Failed 😱

Build Id: 5db92ab6-cdf7-441a-8d17-0832201b6943

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2021-05-28T10:57:39Z

Build Failed 😱

Build Id: 17301a23-5de1-4a9e-8a35-fae22c11f071

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2021-05-28T11:29:57Z

Build Failed 😱

Build Id: fb6e3535-f9a8-4d41-9c71-53a7a3b15637

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2021-05-28T15:55:15Z

Build Failed 😱

Build Id: 9ae4e7dc-9c64-4a1d-957c-67921f6ab46d

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Jake-Shadle · 2021-05-28T16:21:10Z

/assign @markmandel

roberthbailey · 2021-05-29T04:34:26Z

It looks like the sdk conformance tests for rust are failing:

includes/sdk.mk:145: recipe for target 'run-sdk-conformance-no-build' failed
make[2]: *** [run-sdk-conformance-no-build] Error 1

agones-bot · 2021-05-31T13:41:28Z

Build Succeeded 👏

Build Id: 0531b236-0c14-4d51-a7d7-9f8b1c73f7a9

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.15.0-c254850
image: gcr.io/agones-images/agones-ping:1.15.0-c254850
Linux C++ SDK (build): agonessdk-1.15.0-c254850-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.15.0-c254850.zip

A preview of the website (the last 30 builds are retained):

https://c254850-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2112/head:pr_2112 && git checkout pr_2112
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.15.0-c254850

sdks/rust/src/sdk.rs

markmandel

Thanks for the work! This overall looks like an improvement!

My only concern is that this breaks backward compatibility with the previous SDK -- how concerned are we about this? I'm not sure how many users of the Rust SDK we actually have, so it may be a non issue.

Jake-Shadle · 2021-06-15T16:27:50Z

I considered API breakage not super important since the crate has never been published so no API compatibility promises had been made, though I tried not to go too crazy.

markmandel · 2021-06-15T16:38:38Z

I considered API breakage not super important since the crate has never been published so no API compatibility promises had been made, though I tried not to go too crazy.

Yeah, I figure it's probably fine. Also you can always STILL use the previously published code, it doesn't stop working.

@roberthbailey do you have any thoughts on the backward compatibility side of things? I think it's fine, but just want to cover all bases.

roberthbailey · 2021-06-15T21:05:54Z

@roberthbailey do you have any thoughts on the backward compatibility side of things? I think it's fine, but just want to cover all bases.

The only compatibility issues is between the game server and the sdk, which would be caught at compile time. The SDK <-> sidecar should remain the same, so this should be compatible across older and newer Agones releases.

Since you don't need to adopt a new SDK for a new agones release (unless it has new features that you need) I don't see any issues requiring folks to make some changes to their code to adopt a new sdk version. This is actually pretty similar to the process we go through when upgrading client-go .... older versions should work just fine with the k8s apiserver, but when we want the newer code, we pull it in and sometimes need to make changes to our code so that it still compiles and works with the updated libraries.

markmandel · 2021-06-15T21:54:31Z

Since you don't need to adopt a new SDK for a new agones release (unless it has new features that you need) I don't see any issues requiring folks to make some changes to their code to adopt a new sdk version. This is actually pretty similar to the process we go through when upgrading client-go .... older versions should work just fine with the k8s apiserver, but when we want the newer code, we pull it in and sometimes need to make changes to our code so that it still compiles and works with the updated libraries.

Well said! I concur!

So there are some comments above for review - but to add an extra item, we should also update https://agones.dev/site/docs/guides/client-sdks/rust/ to include new documentation for the new SDK.

https://agones.dev/site/docs/contribute/#at-the-page-level feature shortcodes gives you the tools to be able to hide the new documentation until the next release as well.

The logs from CD runs are incredibly hard to read since they are just intermixed with all tests, so I prefixed all of the print messages from the rust example so I can actually read the output more easily.

documentation

Removed the auto health check task since that actually made health checking meaningless since it's _supposed_ to be sent by the application itself when it working properly, not automatically

This method should be better than the previous test/examples as the health check will actually be canceled if the task/thread the health check is owned by is canceled for any reason

agones-bot · 2021-06-24T08:29:06Z

Build Succeeded 👏

Build Id: 48c95b18-c87c-4f58-a44d-5393e299fda1

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.16.0-ed847f3
image: gcr.io/agones-images/agones-ping:1.16.0-ed847f3
Linux C++ SDK (build): agonessdk-1.16.0-ed847f3-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.16.0-ed847f3.zip

A preview of the website (the last 30 builds are retained):

https://ed847f3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2112/head:pr_2112 && git checkout pr_2112
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.16.0-ed847f3

Jake-Shadle · 2021-06-24T08:58:24Z

Sorry, I'm in CET so I rebased just before heading to bed, hopefully you can look at it relatively early your time to get it in, I will only be semi-available tomorrow since it's Midsommar.

google-oss-robot requested review from pooneh-m and roberthbailey May 28, 2021 07:39

google-oss-robot added the size/XXL label May 28, 2021

google-cla bot added the cla: yes label May 28, 2021

Jake-Shadle commented May 28, 2021

View reviewed changes

This was referenced May 28, 2021

Review Rust gRPC ecosystem for Rust SDK #1300

Closed

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

Jake-Shadle force-pushed the rust/replace-grpc-io branch from 6a5d729 to 61a8699 Compare May 28, 2021 10:47

google-oss-robot assigned markmandel May 28, 2021

sawagh added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jun 1, 2021

sawagh removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jun 9, 2021

roberthbailey requested review from markmandel and removed request for roberthbailey and pooneh-m June 10, 2021 06:28

markmandel reviewed Jun 15, 2021

View reviewed changes

sdks/rust/src/sdk.rs Outdated Show resolved Hide resolved

markmandel reviewed Jun 15, 2021

View reviewed changes

sdks/rust/src/sdk.rs Outdated Show resolved Hide resolved

markmandel reviewed Jun 15, 2021

View reviewed changes

Jake-Shadle added 21 commits June 24, 2021 10:03

Allow optional port assignment

2fedfac

Fix field name

4aa3db4

Remove unused generic

defc1ee

Print out error when unable to get game server

46a27e5

Pull out health check into its own separate task

410f818

Update rust-simple example to work with updated SDK

89d5ffd

Remove unused generic parameter

ee05aef

Fix rust tests

410940d

Remove unneeded cruft from rust build image

c30a32c

Remove lines causing excessive rebuilds

dafdd74

Fix rust CI test, add prefix to log messages

82269bd

The logs from CD runs are incredibly hard to read since they are just intermixed with all tests, so I prefixed all of the print messages from the rust example so I can actually read the output more easily.

Address PR feedback

dec667d

Update rust SDK

fd38dda

documentation

spawn_health_task => health_check

d650d4b

Removed the auto health check task since that actually made health checking meaningless since it's _supposed_ to be sent by the application itself when it working properly, not automatically

Use internal timeout for SDK connection

905fdf5

Do continuous health checks

b7a9eba

This method should be better than the previous test/examples as the health check will actually be canceled if the task/thread the health check is owned by is canceled for any reason

Copy protobuffers before build

8b8f291

Bump example version

5df8948

Add trailing newline

776c861

Ensure protos are copied for the test

3982b07

Vendor protos again

ed847f3

Jake-Shadle force-pushed the rust/replace-grpc-io branch from cbdfa68 to ed847f3 Compare June 24, 2021 08:04

markmandel added kind/breaking Breaking change area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Jun 24, 2021

markmandel merged commit 3d88bef into googleforgames:main Jun 24, 2021

davidpdrsn mentioned this pull request Jul 9, 2021

Update to tonic 0.5 and prost 0.8 #2176

Closed

roberthbailey added this to the 1.16.0 milestone Jul 13, 2021

govargo mentioned this pull request Jun 25, 2022

Migrade from github.com/golang/protobuf to google.golang.org/protobuf #2644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace grpcio with tonic #2112

Replace grpcio with tonic #2112

Jake-Shadle commented May 28, 2021

agones-bot commented May 28, 2021

Jake-Shadle May 28, 2021

Jake-Shadle May 28, 2021

Jake-Shadle May 28, 2021

Jake-Shadle May 28, 2021

Jake-Shadle May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

Jake-Shadle commented May 28, 2021

roberthbailey commented May 29, 2021

agones-bot commented May 31, 2021

markmandel left a comment

Jake-Shadle commented Jun 15, 2021

markmandel commented Jun 15, 2021

roberthbailey commented Jun 15, 2021

markmandel commented Jun 15, 2021

agones-bot commented Jun 24, 2021

Jake-Shadle commented Jun 24, 2021

Replace grpcio with tonic #2112

Replace grpcio with tonic #2112

Conversation

Jake-Shadle commented May 28, 2021

agones-bot commented May 28, 2021

Jake-Shadle May 28, 2021

Choose a reason for hiding this comment

Jake-Shadle May 28, 2021

Choose a reason for hiding this comment

Jake-Shadle May 28, 2021

Choose a reason for hiding this comment

Jake-Shadle May 28, 2021

Choose a reason for hiding this comment

Jake-Shadle May 28, 2021

Choose a reason for hiding this comment

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

agones-bot commented May 28, 2021

Jake-Shadle commented May 28, 2021

roberthbailey commented May 29, 2021

agones-bot commented May 31, 2021

markmandel left a comment

Choose a reason for hiding this comment

Jake-Shadle commented Jun 15, 2021

markmandel commented Jun 15, 2021

roberthbailey commented Jun 15, 2021

markmandel commented Jun 15, 2021

agones-bot commented Jun 24, 2021

Jake-Shadle commented Jun 24, 2021