Skip to content

Conversation

lacroixthomas
Copy link
Collaborator

@lacroixthomas lacroixthomas commented Jul 20, 2025

What type of PR is this?
/kind feature

What this PR does / Why we need it:

Added scaffolding for the new binary "processor"

The processor only logs and have a default leader election (under dev feature flag ProcessorAllocator

Which issue(s) this PR fixes:

Part of #4190

Special notes for your reviewer:

@github-actions github-actions bot added size/M kind/feature New features for Agones labels Jul 20, 2025
@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 7c2efa8a-b7d7-4113-86da-7d977bdadcb0

Status: FAILURE

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

@markmandel
Copy link
Collaborator

Just checking before review - should this be in draft? or is it ready for review?

@lacroixthomas
Copy link
Collaborator Author

Just checking before review - should this be in draft? or is it ready for review?

Yep, no need to review, still in draft, still need to do some update and test around the deployment.yaml, adding some default values and also checking the service account, either using the one from the controller or adding a new one, it will need permissions for the leases

Copy link
Collaborator

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review 😄

Copy link
Collaborator

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review 😄

@markmandel
Copy link
Collaborator

Sweet - lemme know if you want to kick it through full CI.

@lacroixthomas
Copy link
Collaborator Author

Perfect, will let you know, might be all good next week for review
Thanks for the quick review though, will update that 😄

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 57b536ec-ee2e-4bec-8f83-89ebbda6538b

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: e18d0f4a-c3f4-4f1b-afb2-d6056814652f

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):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4222/head:pr_4222 && git checkout pr_4222
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.51.0-dev-912dd31

@lacroixthomas lacroixthomas marked this pull request as ready for review July 23, 2025 19:55
@lacroixthomas
Copy link
Collaborator Author

@markmandel
That should be good for review, there is some things as the health check part / metrics, that I would prefer to setup when integrating the allocator inside this agones-processor (from a following PR)

This PR is only to setup the scaffolding of this new binary (with leader election)

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 07907aef-3246-4002-9738-af3be0c2e06e

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):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4222/head:pr_4222 && git checkout pr_4222
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.51.0-dev-100c1bb

@lacroixthomas
Copy link
Collaborator Author

/gcbrun

@lacroixthomas lacroixthomas force-pushed the features/add-processor-scafolding-with-leader-election branch from a24a274 to 838ca62 Compare July 25, 2025 18:25
@markmandel
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just dropped some nits. 👍🏻

@@ -282,6 +286,32 @@ agones:
totalRemoteAllocationTimeout: 30s
allocationBatchWaitTime: 500ms
topologySpreadConstraints: []
processor:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this should be a child of agones.allocator ? Since the processor is only actually part of allocation.

So it would be agones.allocator.processor - then it's contextual, and processor doesn't really make sense on it's own. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep totally make sense, will update that

@@ -0,0 +1,178 @@
# Copyright 2025 Google LLC All Rights Reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file could just be processor.yaml - "deployment" is a bit redundant.

@lacroixthomas
Copy link
Collaborator Author

/gcbrun

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 88b1364a-cab9-4bf2-b365-dbbbde1b6180

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: b722a1a0-3a0a-4c9a-b50f-ae7c6dfa8101

Status: FAILURE

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

@lacroixthomas
Copy link
Collaborator Author

/gcbrun

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 09504aa4-d0d0-4755-a845-099defdaa053

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: b2bfee6f-fbad-422b-9831-9055d50b7a3f

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 943c99fb-2e84-41ca-b7e9-2f99e227e308

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 266a0491-3b34-45ec-8cb7-2bb6edc5a97b

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: fb50fba0-95d8-48b8-bac2-7fa91f6d3a9d

Status: FAILURE

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

@lacroixthomas lacroixthomas force-pushed the features/add-processor-scafolding-with-leader-election branch from 49b1bac to eb00606 Compare July 31, 2025 19:09
@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 6b84e59b-2e4f-4864-8e19-08b6ef3157e2

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):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4222/head:pr_4222 && git checkout pr_4222
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.51.0-dev-77233cb

@lacroixthomas
Copy link
Collaborator Author

/gcbrun

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 0a25737a-6992-458f-b68d-3a31a5aed635

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):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4222/head:pr_4222 && git checkout pr_4222
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.51.0-dev-77233cb

@lacroixthomas lacroixthomas force-pushed the features/add-processor-scafolding-with-leader-election branch from 77233cb to eb00606 Compare August 1, 2025 15:43
@lacroixthomas
Copy link
Collaborator Author

Rolled back the fix of the pipeline to keep it on a separate task

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 2d2b4880-b3fc-4a23-93cd-073870a74149

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):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4222/head:pr_4222 && git checkout pr_4222
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.52.0-dev-d77c96d

Copy link
Collaborator

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good - just some minor questions.

whenLeader(ctx, cancelCtx, logger, conf, kubeClient, func(ctx context.Context) {
logger.Info("Starting processor work as leader")

// Simulate processor work (to ensure the leader is working)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non blocking suggestion)

Suggested change
// Simulate processor work (to ensure the leader is working)
// Simulate processor work (to ensure the leader is working)
// TODO: implement processor work

Just to make it extra clear that more work is coming.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: a9b200f

signals.NewSigTermHandler(func() {
logger.Info("Pod shutdown has been requested, failing readiness check")
cancelCtx()
time.Sleep(1 * time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a minute sleep timer before shutdown? Is this to handle any remaining processing items that are mid flight? (do we care? maybe we do).

If so, should it be configurable like it is in the allocator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably get rid of it from this PR as it's just the scaffolding, it will indeed be configurable on the implementation of the allocator on it 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: a9b200f

@markmandel markmandel enabled auto-merge (squash) August 3, 2025 22:35
@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 51355fed-f058-40f3-8f5b-0ee10106f26a

Status: FAILURE

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

@lacroixthomas
Copy link
Collaborator Author

/gcbrun

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: e4fa21c7-cde5-4051-8e92-372140ef949c

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: c43466a0-d7b3-4b73-a025-ab46ea534957

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):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4222/head:pr_4222 && git checkout pr_4222
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.52.0-dev-b089ea4

@markmandel markmandel merged commit 231024b into googleforgames:main Aug 4, 2025
4 checks passed
@lacroixthomas lacroixthomas mentioned this pull request Aug 13, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones size/L size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants