Add clusterIP for agones-allocator in helm chart #3526

govargo · 2023-12-01T09:40:30Z

What type of PR is this?

/kind feature

What this PR does / Why we need it:

I'd like to use agones-allocator with MultiClusterService(MCS) in multiple Kubernetes clusters.

In order to allocate with multiple clusters environment, currently there are some ways.

External Multi Cluster Gateway API, however, Internal Multi Cluster Gateway API(gke-l7-rilb-mc) doesn't support cross region
Standalone Internal Application LoadBalancer (Internal Application loadbalancer support cross region) outside of GKE
Anthos Service Mesh or other L7 layer proxy. e.g. global-multiplayer-demo uses ASM

I'd like to add another solution, 4. MultiClusterService + client-side load balancing.
This diagram is what I'd like to deploy.

In Kubernetes env, L7 proxy or client-side loadbalancing is needed for gRPC's load balancing.
MultiClusterService is the L4 layer service, so we need client-side loadbalancing, if we'd like to load balancing agones-allocator's gRPC.

e.g.  client-side loadbalancing by golang

conn, err := grpc.DialContext(ctx, allocatorEndpoint, grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`)

To communite by client-side loadbalancing, DNS name resolusion for each agones-allocator pod is needed.
DNS name resolusion for each pod can be done by Headless Service in Kubernetes.
However, current helm chart for agones-allocator doesn't support Headless Service.
So this is why I sent this PR.

Thank you.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

google-oss-prow · 2023-12-01T09:40:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: govargo
Once this PR has been reviewed and has the lgtm label, please assign roberthbailey for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

agones-bot · 2023-12-01T10:46:59Z

Build Succeeded 👏

Build Id: 87e279b9-d9f9-43a7-9281-45d182463b23

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3526/head:pr_3526 && git checkout pr_3526
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-0e2ea03-amd64

markmandel · 2023-12-04T23:40:34Z

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

@@ -72,6 +72,9 @@ spec:
      protocol: TCP
 {{- end }}
  type: {{ .Values.agones.allocator.service.serviceType }}
+{{- if and (eq .Values.agones.allocator.service.serviceType "ClusterIP") (.Values.agones.allocator.service.headless) }}


Rather than a special case for "headless", any reason not to do a agones.allocator.service.clusterIP - which can be set to whatever you want, and provides greater flexibility?

Thank you for your comment!
Surely. The clusterIP field which users can set any value has greater flexibility and can also be used in cases other than headless.

I changed my commit while referring to loadBalanceIP.
{{- $useLoadBalancerIP := and (ne .Values.agones.allocator.service.loadBalancerIP "") (eq .Values.agones.allocator.service.serviceType "LoadBalancer") }}

zmerlynn · 2023-12-04T23:44:33Z

@gongmax FYI

agones-bot · 2023-12-05T08:53:35Z

Build Succeeded 👏

Build Id: 34f90e29-add9-4129-9b79-18b31daaaf13

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

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

https://9f989a9-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3526/head:pr_3526 && git checkout pr_3526
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-9f989a9-amd64

markmandel · 2023-12-05T20:19:08Z

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

@@ -72,6 +73,9 @@ spec:
      protocol: TCP
 {{- end }}
  type: {{ .Values.agones.allocator.service.serviceType }}
+{{- if $clusterIP }}


Suggested change

{{- if $clusterIP }}

{{- if (ne .Values.agones.allocator.service.clusterIP "") }}

I write this because this change sent me lookinga thte K8s reference docs, and looking at Service.spec.clusterIP, and I saw it say:

Only applies to types ClusterIP, NodePort, and LoadBalancer

So it's not just the ClusterIP value for Service.spec.type -- so I'd recommend leaving it open, and let the end user decide when it's appropriate to set the value, rather than constrain them?

Thank you for your suggestion!
It would be better and I updated allocation.yaml and helm.md.

agones-bot · 2023-12-06T09:06:23Z

Build Failed 😱

Build Id: 1445300c-6ead-4022-9c13-73aacee583c9

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

… agones.allocator.service.http.nodePort

govargo · 2023-12-06T09:22:24Z

Step#20 tests passed but make-docker container exited with some reason. I think this is due to flaky.

Step #20 - "tests": ✔✔✔ passed in 664.984153ms
Step #20 - "tests": tested 187 documents
Step #20 - "tests": make[1]: Leaving directory '/workspace/build'
Finished Step #20 - "tests"
ERROR
ERROR: build step 20 "make-docker" failed: step exited with non-zero status: 2
Finished Step #16 - "build-images"

I'll re-test with some update.

agones-bot · 2023-12-06T10:25:34Z

Build Succeeded 👏

Build Id: b054e955-f946-445c-a8ef-4d1d80464ad7

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3526/head:pr_3526 && git checkout pr_3526
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-5c06109-amd64

govargo · 2023-12-06T10:46:26Z

Retest with minor documentation clean-up 5c06109 and passed CI

markmandel

Looks good!

agones-bot · 2023-12-08T02:06:34Z

Build Succeeded 👏

Build Id: bf642237-c600-4333-99ce-8e97a081fe39

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3526/head:pr_3526 && git checkout pr_3526
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-deaf4b1-amd64

google-oss-prow bot requested review from aLekSer and EricFortin December 1, 2023 09:40

google-oss-prow bot added the size/XS label Dec 1, 2023

github-actions bot added the kind/feature New features for Agones label Dec 1, 2023

govargo changed the title ~~Add Headless Service for agones-allocator~~ Add Headless Service for agones-allocator in MultiClusterService Dec 1, 2023

markmandel reviewed Dec 4, 2023

View reviewed changes

govargo force-pushed the allcator-headless-svc branch from 0e2ea03 to 9f989a9 Compare December 5, 2023 07:49

govargo changed the title ~~Add Headless Service for agones-allocator in MultiClusterService~~ Add clusterIP for agones-allocator in helm chart Dec 5, 2023

markmandel reviewed Dec 5, 2023

View reviewed changes

Add clusterIP field for agones-allocator in helm chart

22ea10b

govargo force-pushed the allcator-headless-svc branch from 9f989a9 to 22ea10b Compare December 6, 2023 08:51

Fix indent and order of agones.allocator.service.http.appProtocol and…

5c06109

… agones.allocator.service.http.nodePort

markmandel approved these changes Dec 7, 2023

View reviewed changes

markmandel enabled auto-merge (squash) December 7, 2023 23:57

Merge branch 'main' into allcator-headless-svc

deaf4b1

github-actions bot added the size/S label Dec 7, 2023

markmandel merged commit ee6cd66 into googleforgames:main Dec 8, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clusterIP for agones-allocator in helm chart #3526

Add clusterIP for agones-allocator in helm chart #3526

govargo commented Dec 1, 2023

google-oss-prow bot commented Dec 1, 2023

agones-bot commented Dec 1, 2023

markmandel Dec 4, 2023

govargo Dec 5, 2023 •

edited

Loading

zmerlynn commented Dec 4, 2023

agones-bot commented Dec 5, 2023

markmandel Dec 5, 2023

govargo Dec 6, 2023

agones-bot commented Dec 6, 2023

govargo commented Dec 6, 2023 •

edited

Loading

agones-bot commented Dec 6, 2023

govargo commented Dec 6, 2023

markmandel left a comment

agones-bot commented Dec 8, 2023

	{{- if $clusterIP }}
	{{- if (ne .Values.agones.allocator.service.clusterIP "") }}

Add clusterIP for agones-allocator in helm chart #3526

Add clusterIP for agones-allocator in helm chart #3526

Conversation

govargo commented Dec 1, 2023

google-oss-prow bot commented Dec 1, 2023

agones-bot commented Dec 1, 2023

markmandel Dec 4, 2023

Choose a reason for hiding this comment

govargo Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

zmerlynn commented Dec 4, 2023

agones-bot commented Dec 5, 2023

markmandel Dec 5, 2023

Choose a reason for hiding this comment

govargo Dec 6, 2023

Choose a reason for hiding this comment

agones-bot commented Dec 6, 2023

govargo commented Dec 6, 2023 • edited Loading

agones-bot commented Dec 6, 2023

govargo commented Dec 6, 2023

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented Dec 8, 2023

govargo Dec 5, 2023 •

edited

Loading

govargo commented Dec 6, 2023 •

edited

Loading