Allow the ports for gRPC and REST to be configured for the allocator service #2272

roberthbailey · 2021-09-21T07:50:13Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking

/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind hotfix

What this PR does / Why we need it: Makes it possible to disable the REST endpoint in the allocator service or to run gRPC and REST on different ports. This allows using the non-experimental gRPC server code.

Which issue(s) this PR fixes:

Closes #2263

Special notes for your reviewer:

~~This is currently a work-in-progress. I still need to update the documentation before this can be merged.~~

I've added documentation.

This is a breaking change because I'm renaming some helm parameters (which can be seen in the updates to the Makefile and the allocator service documentation). In the 1.18.0 release we should notify folks that if they rely on these helm parameters that they now have different names.

agones-bot · 2021-09-21T08:18:04Z

Build Succeeded 👏

Build Id: 9b245954-6b49-4557-bc11-16f9c4f60ef5

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2272/head:pr_2272 && git checkout pr_2272
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-f04224c

roberthbailey · 2021-09-21T19:24:12Z

install/helm/agones/templates/service/allocation.yaml

 {{- end }}
 spec:
  selector:
    multicluster.agones.dev/role: allocator
  ports:
-    - port: {{ .Values.agones.allocator.http.port }}
+{{- if .Values.agones.allocator.service.http.enabled }}


The logic here is similar to what is in cmd/allocator/main.go but is a bit harder to read due to the templating language.

There are 4 cases to cover:

If http & grpc are both enabled and use the same port, only configure the http port

If http & grpc are both enabled and use different ports, configure the http port and the grpc port

If only http is enabled, only configure the http port

If only grpc is enabled, only configure the grpc port

The logic to cover the 4 cases has three branches:

If the http port is enabled, then use it (covers cases 1-3 & skipping covers 4)

If the grpc port is also enabled and it is different from the http port, use it as well (case 2)

If the http port is disabled but the grpc port is enabled, use it (case 4)

Locally I tested this by modifying my values.yaml file and running make gen-install to see the resulting yaml and verify that the correct number or ports were defined (1 or 2) with the correct values.

agones-bot · 2021-09-22T05:57:13Z

Build Failed 😱

Build Id: 3a5fe140-13ad-4480-abe6-fc1fcf3b5b56

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

agones-bot · 2021-09-22T06:06:20Z

Build Succeeded 👏

Build Id: f2a04482-3eaf-4f29-b899-09ad9d4e329e

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

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

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

https://691306d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2272/head:pr_2272 && git checkout pr_2272
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-691306d

agones-bot · 2021-09-23T17:30:40Z

Build Succeeded 👏

Build Id: 6992a215-2a4a-4552-8aa4-f7ebd8b4e6f2

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

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

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

https://56f6f57-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2272/head:pr_2272 && git checkout pr_2272
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-56f6f57

rcreasey

Leaving feedback, but not requiring them to be changed for the PR.

rcreasey · 2021-09-23T22:38:48Z

install/helm/agones/templates/service/allocation.yaml

@@ -161,8 +184,19 @@ spec:
        - name: FEATURE_GATES
          value: {{ .Values.agones.featureGates | quote }}
        ports:
+        {{- if .Values.agones.allocator.service.http.enabled }}
        - name: https


I think that name should also be a changeable attribute if we're already changing the port values. There are some use cases where changing the port to 80 and leaving the name as https would be disadvantageous. If not, then perhaps we should statically name this to just be http to be consistent with the nomenclature of the helm values label.

rcreasey · 2021-09-23T22:40:06Z

install/helm/agones/templates/service/allocation.yaml

+          containerPort: {{ .Values.agones.allocator.service.http.targetPort }}
+        {{- if .Values.agones.allocator.service.grpc.enabled }}
+        {{- if ne .Values.agones.allocator.service.grpc.port .Values.agones.allocator.service.http.port }}
+        - name: grpc


Related to the above, it might be worth having this be defined via the helm value as well.

install/helm/agones/templates/service/allocation.yaml

agones-bot · 2021-09-24T05:08:53Z

Build Failed 😱

Build Id: dba18489-a5d6-4acc-8d6c-2fb08b16a49e

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

agones-bot · 2021-09-24T05:40:17Z

Build Succeeded 👏

Build Id: c4da79f8-b978-4d1f-90f0-45792e44c72b

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2272/head:pr_2272 && git checkout pr_2272
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-ce8ffd1

markmandel

Couple of nits on logging, but otherwise LGTM

markmandel · 2021-09-24T17:52:14Z

cmd/allocator/main.go

@@ -92,6 +91,10 @@ func main() {
 		runtime.SetLevel(logrus.InfoLevel)
 	}

+	if !validPort(conf.GRPCPort) && !validPort(conf.HTTPPort) {
+		logger.Fatalf("Must specify a valid gRPC port or an HTTP port for the allocator service. grpc port = %d, http port = %d", conf.GRPCPort, conf.HTTPPort)


Minor nit:

Suggested change

logger.Fatalf("Must specify a valid gRPC port or an HTTP port for the allocator service. grpc port = %d, http port = %d", conf.GRPCPort, conf.HTTPPort)

logger.WithField("grpc-port", conf.GRPCPort).WithField("http-port", conf.HTTPPort).Fatal("Must specify a valid gRPC port or an HTTP port for the allocator service")

Since we have structured logging.

markmandel · 2021-09-24T18:32:45Z

cmd/allocator/main.go

+}
+
+func runREST(h *serviceHandler, httpPort int) {
+	logger.Infof("Running the rest handler on port %d", httpPort)


Suggested change

logger.Infof("Running the rest handler on port %d", httpPort)

logger.WithField("port", httpPort).Info("Running the rest handler")

Nit to prefer a structured log 😄

markmandel · 2021-09-24T18:33:18Z

cmd/allocator/main.go

-	err = http.ListenAndServe(":8080", http.DefaultServeMux)
-	logger.WithError(err).Fatal("allocation service crashed")
+func runGRPC(h *serviceHandler, grpcPort int) {
+	logger.Infof("Running the grpc handler on port %d", grpcPort)


Suggested change

logger.Infof("Running the grpc handler on port %d", grpcPort)

logger.WithField("port", grpcPort).Info("Running the grpc handler on port")

service.

agones-bot · 2021-09-24T21:27:40Z

Build Succeeded 👏

Build Id: b8475669-4045-4091-ac52-5c03d7749439

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2272/head:pr_2272 && git checkout pr_2272
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-e722921

agones-bot · 2021-09-24T21:42:20Z

Build Failed 😱

Build Id: c94a3e86-132d-44fd-8897-03092274cf82

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

markmandel

Flake:

--- FAIL: TestFleetRollingUpdate (0.00s)
    --- FAIL: TestFleetRollingUpdate/Use_fleet_Patch_false_10% (118.20s)
        fleet_test.go:331: 
            	Error Trace:	fleet_test.go:331
            	Error:      	Received unexpected error:
            	            	new replicas should be less than target + maxSurge + maxUnavailable + shift. Replicas: 11, Expected: 10
            	Test:       	TestFleetRollingUpdate/Use_fleet_Patch_false_10%

Retrying!

google-oss-robot · 2021-09-24T22:00:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, rcreasey, 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~~ [markmandel,roberthbailey]

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

agones-bot · 2021-09-24T22:27:03Z

Build Succeeded 👏

Build Id: 720b6260-928d-4f00-9bd7-0647d9e35da1

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

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

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

https://8a23675-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2272/head:pr_2272 && git checkout pr_2272
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-8a23675

google-cla bot added the cla: yes label Sep 21, 2021

google-oss-robot added the size/L label Sep 21, 2021

google-oss-robot requested review from markmandel and pooneh-m September 21, 2021 07:50

google-oss-robot added the approved label Sep 21, 2021

roberthbailey mentioned this pull request Sep 21, 2021

Allow the allocator service to use the go http/2 server #2263

Closed

roberthbailey commented Sep 21, 2021

View reviewed changes

roberthbailey force-pushed the allocator-service branch from 691306d to 56f6f57 Compare September 22, 2021 05:22

roberthbailey added kind/breaking Breaking change kind/feature New features for Agones area/operations Installation, updating, metrics etc labels Sep 22, 2021

rcreasey reviewed Sep 23, 2021

View reviewed changes

roberthbailey force-pushed the allocator-service branch from 56f6f57 to ce8ffd1 Compare September 24, 2021 04:38

roberthbailey mentioned this pull request Sep 24, 2021

Skip volume mounts in the allocator pod if TLS on mTLS is disabled #2276

Closed

rcreasey approved these changes Sep 24, 2021

View reviewed changes

google-oss-robot assigned rcreasey Sep 24, 2021

google-oss-robot added the lgtm label Sep 24, 2021

markmandel reviewed Sep 24, 2021

View reviewed changes

roberthbailey force-pushed the allocator-service branch from ce8ffd1 to e722921 Compare September 24, 2021 21:01

google-oss-robot removed the lgtm label Sep 24, 2021

roberthbailey added 4 commits September 24, 2021 21:14

Allow the ports for gRPC and REST to be configured for the allocator

1361b9c

service.

Ran make gen-install

06d8db3

Updated documentation for the allocator service.

38c33a4

Make the port name used by the allocator service a variable as well.

8a23675

roberthbailey force-pushed the allocator-service branch from e722921 to 8a23675 Compare September 24, 2021 21:14

markmandel approved these changes Sep 24, 2021

View reviewed changes

google-oss-robot assigned markmandel Sep 24, 2021

google-oss-robot added the lgtm label Sep 24, 2021

markmandel added this to the 1.18.0 milestone Sep 24, 2021

markmandel merged commit fbe5380 into googleforgames:main Sep 24, 2021

roberthbailey mentioned this pull request Sep 24, 2021

Allocator gRPC doesn't work without TLS #1945

Closed

SpringMT mentioned this pull request Feb 9, 2022

docs: Support gRPC without TLS #2472

Merged

roberthbailey deleted the allocator-service branch March 28, 2022 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the ports for gRPC and REST to be configured for the allocator service #2272

Allow the ports for gRPC and REST to be configured for the allocator service #2272

roberthbailey commented Sep 21, 2021 •

edited

Loading

agones-bot commented Sep 21, 2021

roberthbailey Sep 21, 2021

agones-bot commented Sep 22, 2021

agones-bot commented Sep 22, 2021

agones-bot commented Sep 23, 2021

rcreasey left a comment

rcreasey Sep 23, 2021

roberthbailey Sep 24, 2021

rcreasey Sep 23, 2021

roberthbailey Sep 24, 2021

agones-bot commented Sep 24, 2021

agones-bot commented Sep 24, 2021

markmandel left a comment

markmandel Sep 24, 2021

roberthbailey Sep 24, 2021

markmandel Sep 24, 2021

roberthbailey Sep 24, 2021

markmandel Sep 24, 2021

roberthbailey Sep 24, 2021

agones-bot commented Sep 24, 2021

agones-bot commented Sep 24, 2021

markmandel left a comment

google-oss-robot commented Sep 24, 2021

agones-bot commented Sep 24, 2021

	logger.Fatalf("Must specify a valid gRPC port or an HTTP port for the allocator service. grpc port = %d, http port = %d", conf.GRPCPort, conf.HTTPPort)
	logger.WithField("grpc-port", conf.GRPCPort).WithField("http-port", conf.HTTPPort).Fatal("Must specify a valid gRPC port or an HTTP port for the allocator service")

	logger.Infof("Running the rest handler on port %d", httpPort)
	logger.WithField("port", httpPort).Info("Running the rest handler")

	logger.Infof("Running the grpc handler on port %d", grpcPort)
	logger.WithField("port", grpcPort).Info("Running the grpc handler on port")

Allow the ports for gRPC and REST to be configured for the allocator service #2272

Allow the ports for gRPC and REST to be configured for the allocator service #2272

Conversation

roberthbailey commented Sep 21, 2021 • edited Loading

agones-bot commented Sep 21, 2021

Choose a reason for hiding this comment

agones-bot commented Sep 22, 2021

agones-bot commented Sep 22, 2021

agones-bot commented Sep 23, 2021

rcreasey left a 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

agones-bot commented Sep 24, 2021

agones-bot commented Sep 24, 2021

markmandel left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Sep 24, 2021

agones-bot commented Sep 24, 2021

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Sep 24, 2021

agones-bot commented Sep 24, 2021

roberthbailey commented Sep 21, 2021 •

edited

Loading