Updates GRPC to v1.20.1 #1144

cyriltovena · 2019-10-24T17:06:28Z

This version is the one used by the latest release of OpenCensus which is why I made this PR.

I've also update protobuf to the same version as OpenCensus and grpc-gateway.

google-oss-robot · 2019-10-24T17:06:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cyriltovena

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~~ [cyriltovena]

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

markmandel · 2019-10-24T17:13:11Z

/cc @aLekSer does this solve your issue with #1106 ?

agones-bot · 2019-10-24T17:13:23Z

Build Failed 😱

Build Id: d0c3445f-7be8-44ea-bb7d-f3f32d4127cb

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

markmandel · 2019-10-24T17:16:32Z

Not 100% sure why this is failing. If it's the cpp build cache, may need to adjust the _CPP_SDK_BUILD_CACHE_KEY in cloudbuild.yaml to hash a file or just add a -v2 to it or something.

cyriltovena · 2019-10-24T17:17:41Z

failing too locally but it wasn't earlier. I'll investigate.

aLekSer · 2019-10-25T09:06:12Z

@markmandel This solves sdk.swagger.json definition file. I need to make a fix of the HTTP Api test.
However I could not test the resulting schema, because for now there is an issue with x-stream-definitions in both swagger generators I have tried (details are in that issue).

aLekSer · 2019-10-25T13:12:17Z

Thanks for an update. So this PR would also break new HTTP test which uses swagger generator.

$ make  run-sdk-conformance-test SDK_FOLDER=restapi HTTP_PORT=9050    
... [lines omitted]                     
{"httpEndpoint":":9050","message":"Starting SDKServer grpc-gateway...","severity":"info","source":"main","time":"2019-10-25T13:06:06.3917168Z"}                                                          
# agones.dev/agones/test/sdk/restapi/swagger
swagger/api_sdk.go:742:63: undefined: XStreamDefinitionssdkGameServer
swagger/api_sdk.go:748:23: undefined: XStreamDefinitionssdkGameServer
swagger/api_sdk.go:806:10: undefined: XStreamDefinitionssdkGameServer
make[2]: *** [run-sdk-command] Error 2
{"message":"shutting down sdk server","severity":"info","source":"main","time":"2019-10-25T13:06:36.358519Z"}                                                                                            
{"message":"Compare","severity":"info","time":"2019-10-25T13:06:36.3586729Z"}
{"message":"Testing Failed [ready allocate setlabel setannotation gameserver health shutdown watch reserve] []","severity":"info","time":"2019-10-25T13:06:36.358818Z"}                                  
make[1]: *** [run-sdk-conformance-no-build] Error 1
make: *** [run-sdk-conformance-test] Error 2

I am thinking on how we can fix that.
More details and analysis of the problem and possible solutions could be found here #1079 .
For now we can skip testing watch.

cyriltovena · 2019-10-28T13:19:38Z

I'm struggling with the cpp SDK. I'm backtracking my changes one by one and rebuilding to see what's wrong. (And building when I have time, cpp build is horribly slow.)

markmandel · 2019-11-04T20:02:19Z

@cyriltovena did you want a hand with this? I can take a stab, if you would like?

markmandel · 2019-11-05T01:29:51Z

So I rebased to master on my local branch, and ran the compilation script. I just wanted to capture the output, so we can start to take it apart:

Here it is:
https://gist.github.com/markmandel/a6a000491740fff08fb05d9da00910e8

markmandel · 2019-11-05T18:03:09Z

I'm not 100% sure, but it seems like my research is pointing me to something like this:
protocolbuffers/protobuf#5998

For C++ the version of protoc used to generate the pb.cc and pb.h files has to exactly match the version of libprotobuf you link against. It looks like your code is mixing different versions, probably an old version of the generated code with a newer version of the protobuf library.

Will run around the code, and see if we have a definition of protoc that is also included elsewhere that also need to be updated somewhere (unless someone else beats me to it)

cyriltovena · 2019-11-05T23:38:43Z

yes this is the problem. The cmake has a version that he uses to get source code https://github.com/googleforgames/agones/blob/master/sdks/cpp/CMakeLists.txt#L98 and build against it.

agones-bot · 2019-11-05T23:52:42Z

Build Failed 😱

Build Id: 05416608-6cb5-4958-9200-9790fcfdd596

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

agones-bot · 2019-11-05T23:55:55Z

Build Failed 😱

Build Id: 1c67d83c-5dfd-4ef6-ac02-ab4a2f66722b

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

agones-bot · 2019-11-06T00:34:19Z

Build Failed 😱

Build Id: 8da1a53a-9c98-4c86-820f-d791310e57c6

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

markmandel · 2019-11-07T02:27:03Z

Kicking off a retry, see if that helps.

markmandel · 2019-11-07T23:03:29Z

... I think this is timing out!

agones-bot · 2019-11-07T23:17:04Z

Build Failed 😱

Build Id: 73d7134f-e784-4cd8-9d01-7d3f6515a044

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

markmandel · 2019-11-07T23:21:31Z

Since it seems like this is not going to build on Cloud Build - should I build it on my workstation (I may have it already), and push it up to gcr.io?

markmandel · 2019-11-21T01:36:02Z

@cyriltovena if you want to resolve the conflicts, I'm happy to build the image on my workstation and push this up.

cyriltovena · 2019-11-23T14:39:55Z

Will do

cyriltovena · 2019-11-24T01:20:23Z

While 1.20.1 is the last version used by OpenCensus, OpenTelemetry uses 1.24, I'm thinking while we are at it and since GRPC is backward compatible should we move to 1.24 instead ? 🤷‍♂

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

cyriltovena · 2019-11-27T18:50:36Z

@markmandel Can you let me know if you can build locally ? You have to start with a clean setup.

markmandel · 2019-12-02T20:15:08Z

Sure! I'll try and get on it today. Not sure if it's worth the experiment, but you could also change the name of the CPP build cache, which would force a clean build on Cloud Build.

markmandel · 2019-12-03T18:41:29Z

...building on my workstation!

markmandel · 2019-12-03T19:55:38Z

So I just realised something - I can build the image, but the image isn't the issue (we don't even store the cpp sdk build image) - the issue is to have the cpp build cache - is it not?

Can you edit https://github.com/cyriltovena/agones/blob/grpc/cloudbuild.yaml#L280 to make it something like "cpp-sdk-build-1" or even edit the build steps to include a checksum of https://github.com/cyriltovena/agones/blob/grpc/sdks/cpp/CMakeLists.txt (might be a good option).

See if things build from there?

aLekSer · 2019-12-04T11:01:58Z

As I mentioned above there is an issue with swagger codegen:
make run-sdk-conformance-test SDK_FOLDER=restapi HTTP_PORT=9050 is failing because of
swagger/api_sdk.go:742:63: undefined: XStreamDefinitionssdkGameServer
I am working on adding new version of the test which would override this issue.

cyriltovena · 2019-12-06T16:11:36Z

the issue is to have the cpp build cache - is it not?

Yes works for me locally.

No idea how to use a cmakefile. Sorry.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

agones-bot · 2019-12-06T16:50:33Z

Build Failed 😱

Build Id: 60622269-38fb-456c-8ed7-5a22d143dd58

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

cyriltovena · 2019-12-06T17:01:05Z

alright I sinked enough time into this. I'm done !

cyriltovena · 2019-12-06T17:01:41Z

also quite mad about C++.

markmandel · 2019-12-06T17:11:38Z

Okay, I'm going to take this over. We will make this work!

markmandel · 2019-12-06T17:21:22Z

So I'm digging into this - it looks like it did actually compile correctly, but it failed on @aLekSer 's afformentioned issue:

# agones.dev/agones/test/sdk/restapi/swagger
swagger/api_sdk.go:742:63: undefined: XStreamDefinitionssdkGameServer
swagger/api_sdk.go:748:23: undefined: XStreamDefinitionssdkGameServer
swagger/api_sdk.go:806:10: undefined: XStreamDefinitionssdkGameServer
includes/sdk.mk:84: recipe for target 'run-sdk-command' failed
make[3]: *** [run-sdk-command] Error 2
{"grpcEndpoint":":9357","message":"Starting SDKServer grpc service...","severity":"info","source":"main","time":"2019-12-06T16:48:18.569904778Z"}
{"httpEndpoint":":9050","message":"Starting SDKServer grpc-gateway...","severity":"info","source":"main","time":"2019-12-06T16:48:18.570635165Z"}
{"message":"shutting down sdk server","severity":"info","source":"main","time":"2019-12-06T16:48:48.569699733Z"}
{"message":"Compare","severity":"info","time":"2019-12-06T16:48:48.569899218Z"}
{"message":"Testing Failed [ready allocate setlabel setannotation gameserver health shutdown watch reserve] []","severity":"info","time":"2019-12-06T16:48:48.569943486Z"}

@aLekSer when do you think this will be updated?

Taking over work started in googleforgames#1144, so we can upgrade Opencensus - Updates gRPC in all places, which updates proto and grpc-gateway - Regenerates the code for all gRPC SDKs as well - Need to workaround a issue with the /watch Swagger codegen, as the swagger changed between versions. This likely means the JSON structure from grpc-gateway for streaming operations also changed. This is a breaking change, but we have no control over it (not sure if anyone is using this specific REST api endpoint, so it's probably a non issue). - Use the C++ library requisites file in the cloud build cache key, so it gets updated as dependencies get updated. Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

Taking over work started in googleforgames#1144, so we can upgrade Opencensus - Updates gRPC in all places, which updates proto and grpc-gateway - Regenerates the code for all gRPC SDKs as well - Need to workaround a issue with the /watch Swagger codegen, as the swagger changed between versions. This likely means the JSON structure from grpc-gateway for streaming operations also changed. This is a breaking change, but we have no control over it (not sure if anyone is using this specific REST api endpoint, so it's probably a non issue). - Use the C++ library requisites file in the cloud build cache key, so it gets updated as dependencies get updated. Closes googleforgames#1214 Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

Taking over work started in #1144, so we can upgrade Opencensus - Updates gRPC in all places, which updates proto and grpc-gateway - Regenerates the code for all gRPC SDKs as well - Need to workaround a issue with the /watch Swagger codegen, as the swagger changed between versions. This likely means the JSON structure from grpc-gateway for streaming operations also changed. This is a breaking change, but we have no control over it (not sure if anyone is using this specific REST api endpoint, so it's probably a non issue). - Use the C++ library requisites file in the cloud build cache key, so it gets updated as dependencies get updated. Closes #1214 Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

Taking over work started in googleforgames#1144, so we can upgrade Opencensus - Updates gRPC in all places, which updates proto and grpc-gateway - Regenerates the code for all gRPC SDKs as well - Need to workaround a issue with the /watch Swagger codegen, as the swagger changed between versions. This likely means the JSON structure from grpc-gateway for streaming operations also changed. This is a breaking change, but we have no control over it (not sure if anyone is using this specific REST api endpoint, so it's probably a non issue). - Use the C++ library requisites file in the cloud build cache key, so it gets updated as dependencies get updated. Closes googleforgames#1214 Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>

cyriltovena added kind/cleanup Refactoring code, fixing up documentation, etc area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. labels Oct 24, 2019

google-oss-robot requested review from ilkercelikyilmaz and pooneh-m October 24, 2019 17:06

google-oss-robot added approved size/XXL labels Oct 24, 2019

markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 24, 2019

markmandel removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 29, 2019

cyriltovena added 8 commits November 24, 2019 12:35

Update GRPC to v1.20.1

ec4a649

Update GRPC to v1.20.1

74a36cc

update cmake prerequistes to use the env variable.

c98c79e

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

go mod vendor

2ce1a34

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

bump the sha of the base dockerfile to force a rebuild

3d3bad5

Fixed conflict and regen everything.

a451922

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

go mod verify, vendor.

3570bad

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Update vendor and generated files

46371b4

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

cyriltovena force-pushed the grpc branch from 5cf4530 to 46371b4 Compare November 25, 2019 17:02

markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Dec 4, 2019

Attempt to remove the CI cache.

fc401bd

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

cyriltovena closed this Dec 6, 2019

markmandel mentioned this pull request Dec 6, 2019

Updates GRPC to v1.20.1 #1214

Closed

markmandel mentioned this pull request Dec 7, 2019

Update GRPC to v1.20.1 #1215

Merged

SaitejaTamma removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates GRPC to v1.20.1 #1144

Updates GRPC to v1.20.1 #1144

cyriltovena commented Oct 24, 2019 •

edited

Loading

google-oss-robot commented Oct 24, 2019

markmandel commented Oct 24, 2019

agones-bot commented Oct 24, 2019

markmandel commented Oct 24, 2019

cyriltovena commented Oct 24, 2019

aLekSer commented Oct 25, 2019 •

edited

Loading

aLekSer commented Oct 25, 2019 •

edited

Loading

cyriltovena commented Oct 28, 2019

markmandel commented Nov 4, 2019

markmandel commented Nov 5, 2019

markmandel commented Nov 5, 2019

cyriltovena commented Nov 5, 2019

agones-bot commented Nov 5, 2019

agones-bot commented Nov 5, 2019

agones-bot commented Nov 6, 2019

markmandel commented Nov 7, 2019

markmandel commented Nov 7, 2019

agones-bot commented Nov 7, 2019

markmandel commented Nov 7, 2019

markmandel commented Nov 21, 2019

cyriltovena commented Nov 23, 2019

cyriltovena commented Nov 24, 2019

cyriltovena commented Nov 27, 2019

markmandel commented Dec 2, 2019

markmandel commented Dec 3, 2019

markmandel commented Dec 3, 2019 •

edited

Loading

aLekSer commented Dec 4, 2019

cyriltovena commented Dec 6, 2019 •

edited

Loading

agones-bot commented Dec 6, 2019

cyriltovena commented Dec 6, 2019

cyriltovena commented Dec 6, 2019

markmandel commented Dec 6, 2019

markmandel commented Dec 6, 2019

Updates GRPC to v1.20.1 #1144

Updates GRPC to v1.20.1 #1144

Conversation

cyriltovena commented Oct 24, 2019 • edited Loading

google-oss-robot commented Oct 24, 2019

markmandel commented Oct 24, 2019

agones-bot commented Oct 24, 2019

markmandel commented Oct 24, 2019

cyriltovena commented Oct 24, 2019

aLekSer commented Oct 25, 2019 • edited Loading

aLekSer commented Oct 25, 2019 • edited Loading

cyriltovena commented Oct 28, 2019

markmandel commented Nov 4, 2019

markmandel commented Nov 5, 2019

markmandel commented Nov 5, 2019

cyriltovena commented Nov 5, 2019

agones-bot commented Nov 5, 2019

agones-bot commented Nov 5, 2019

agones-bot commented Nov 6, 2019

markmandel commented Nov 7, 2019

markmandel commented Nov 7, 2019

agones-bot commented Nov 7, 2019

markmandel commented Nov 7, 2019

markmandel commented Nov 21, 2019

cyriltovena commented Nov 23, 2019

cyriltovena commented Nov 24, 2019

cyriltovena commented Nov 27, 2019

markmandel commented Dec 2, 2019

markmandel commented Dec 3, 2019

markmandel commented Dec 3, 2019 • edited Loading

aLekSer commented Dec 4, 2019

cyriltovena commented Dec 6, 2019 • edited Loading

agones-bot commented Dec 6, 2019

cyriltovena commented Dec 6, 2019

cyriltovena commented Dec 6, 2019

markmandel commented Dec 6, 2019

markmandel commented Dec 6, 2019

cyriltovena commented Oct 24, 2019 •

edited

Loading

aLekSer commented Oct 25, 2019 •

edited

Loading

aLekSer commented Oct 25, 2019 •

edited

Loading

markmandel commented Dec 3, 2019 •

edited

Loading

cyriltovena commented Dec 6, 2019 •

edited

Loading