Implement converters between the GameServerAllocation API and allocation.proto #1117

pooneh-m · 2019-10-16T00:43:42Z

Allocation.proto will be the external facing API for agones-allocator service. To use the allocation logic, the service needs to converts between external API (e.g allocation.proto) and internal API (e.g. GameServerAllocation).

This change includes:

Converters implementations:
- GSA to allocation.proto converter APIs are used by allocator library to make a remote allocation call
- allocation.proto to GSA converter APIs are used by agones-allocator service to call the allocator.allocate() API
Moving the generated code to v1alpha1 to make it versioned.

agones-bot · 2019-10-16T01:04:46Z

Build Failed 😱

Build Id: 36d4daa2-3b0e-45da-b6b1-ecc27c9db2ff

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

agones-bot · 2019-10-16T18:11:15Z

Build Succeeded 👏

Build Id: d75fa1c3-14e9-4061-8855-99e812b503b4

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

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

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

https://5ddbfc7-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1117/head:pr_1117 && git checkout pr_1117
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-5ddbfc7

pkg/allocation/libs/converter.go

roberthbailey · 2019-10-16T19:39:12Z

pkg/allocation/libs/converter.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// ConvertAllocationRequestV1Alpha1ToGSAV1 converts AllocationRequest to GameServerAllocation V1 (GSA)


Is GSA v1 the "canonical" form? When we introduce v1beta1 or v1 for the allocation api how much more conversion code will we need to add?

If we diverge to multiple versions of GSA and allocation.proto, there may be multiple conversions. However, ideally the conversions will always happen to the latest GSA and we handle compatibility at this layer. In that case there should be an internal version of GSA.

roberthbailey · 2019-10-16T19:42:07Z

pkg/allocation/libs/converter.go

+// limitations under the License.
+
+// Package converters includes API concersions between GameServerAllocation API and the Allocation proto APIs
+package converters


It looks like this was all done by hand. But the fields in the proto and the k8s api look very similar. Is there a reason that we don't auto-generate the service definition from the api spec (to reduce the amount of manual conversion code)?

I looked into proteus. There are some issues generating proto from golang package due to old packages and mismatches, but it seems to be eventually doable. Now I am thinking how much value do we get by reusing Spec and Status. Here are the pros and cons for not auto generating part of API from GSA:

pros:

Separating external facing APIs and internal APIs for agones-allocator. The external API is allocation.proto and the internal API is GSA. Internal API may change, but the external API should not change after a stable release.

The allocation.proto fields will follow best practices for REST and gRPC API design and GSA will follow the k8s API design. So for example in the case of status, I am planning to drop states (Proposal: Allocator service to return 400+ http status for failure #1040) for contention and unallocated and returning the expected gRPC errors. There are also field names that are different between the two e.g. preferredGameServerSelectors vs preferred.

proto generation and keeping dependency in sync with proteus seems to be a hassle (I still haven't got it working)

Cons:

There are more maintenance e.g. writing more code for conversion layer and making sure the API is in sync.

I am inclined to keep the APIs as is partly because of the benefits I explain which I feel outweights the disadvantages, and partly because I am biased at this stage :)
Let me know if you have a strong opinion on generating the proto.

agones-bot · 2019-10-16T22:57:43Z

Build Failed 😱

Build Id: 23f4668d-17ea-40a2-a2fe-ecedc1537719

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

…ion PROTO API

agones-bot · 2019-10-16T23:39:25Z

Build Succeeded 👏

Build Id: e77f53df-4d81-49fa-bee6-c6c65cf6c4cf

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

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

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

https://5750bcb-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1117/head:pr_1117 && git checkout pr_1117
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-5750bcb

agones-bot · 2019-10-17T00:01:49Z

Build Succeeded 👏

Build Id: 0ffa635a-e1ce-4160-8839-73dfaf18486c

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

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

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

https://0c5b81e-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1117/head:pr_1117 && git checkout pr_1117
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-0c5b81e

google-oss-robot · 2019-10-17T19:05:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pooneh-m, roberthbailey

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~~ [pooneh-m,roberthbailey]

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

google-oss-robot added the size/L label Oct 16, 2019

google-oss-robot requested review from ilkercelikyilmaz and markmandel October 16, 2019 00:43

google-oss-robot added the approved label Oct 16, 2019

pooneh-m removed request for markmandel and ilkercelikyilmaz October 16, 2019 00:55

pooneh-m force-pushed the converter branch from 3832d2c to 5ddbfc7 Compare October 16, 2019 17:40

google-oss-robot added size/XL and removed size/L labels Oct 16, 2019

pooneh-m requested a review from roberthbailey October 16, 2019 18:13

roberthbailey reviewed Oct 16, 2019

View reviewed changes

pooneh-m force-pushed the converter branch from 5ddbfc7 to 5750bcb Compare October 16, 2019 22:44

Implement convertors between the GameServerAllocation API and allocat…

0c5b81e

…ion PROTO API

pooneh-m force-pushed the converter branch from 5750bcb to 0c5b81e Compare October 16, 2019 23:30

pooneh-m requested a review from roberthbailey October 17, 2019 16:50

pooneh-m mentioned this pull request Oct 17, 2019

Change the API for allocator to allocation.proto #1123

Merged

roberthbailey approved these changes Oct 17, 2019

View reviewed changes

google-oss-robot assigned roberthbailey Oct 17, 2019

google-oss-robot added the lgtm label Oct 17, 2019

roberthbailey merged commit 17912d1 into googleforgames:master Oct 17, 2019

markmandel added this to the 1.1.0 milestone Oct 22, 2019

markmandel added the kind/feature New features for Agones label Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement converters between the GameServerAllocation API and allocation.proto #1117

Implement converters between the GameServerAllocation API and allocation.proto #1117

pooneh-m commented Oct 16, 2019 •

edited

Loading

agones-bot commented Oct 16, 2019

agones-bot commented Oct 16, 2019

roberthbailey Oct 16, 2019

pooneh-m Oct 16, 2019

roberthbailey Oct 16, 2019

pooneh-m Oct 16, 2019

agones-bot commented Oct 16, 2019

agones-bot commented Oct 16, 2019

agones-bot commented Oct 17, 2019

google-oss-robot commented Oct 17, 2019

Implement converters between the GameServerAllocation API and allocation.proto #1117

Implement converters between the GameServerAllocation API and allocation.proto #1117

Conversation

pooneh-m commented Oct 16, 2019 • edited Loading

agones-bot commented Oct 16, 2019

agones-bot commented Oct 16, 2019

roberthbailey Oct 16, 2019

Choose a reason for hiding this comment

pooneh-m Oct 16, 2019

Choose a reason for hiding this comment

roberthbailey Oct 16, 2019

Choose a reason for hiding this comment

pooneh-m Oct 16, 2019

Choose a reason for hiding this comment

agones-bot commented Oct 16, 2019

agones-bot commented Oct 16, 2019

agones-bot commented Oct 17, 2019

google-oss-robot commented Oct 17, 2019

pooneh-m commented Oct 16, 2019 •

edited

Loading