-
Notifications
You must be signed in to change notification settings - Fork 858
Feat: Add new binary processor #4222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Add new binary processor #4222
Conversation
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review 😄
Sweet - lemme know if you want to kick it through full CI. |
Perfect, will let you know, might be all good next week for review |
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. |
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:
|
@markmandel This PR is only to setup the scaffolding of this new binary (with leader election) |
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:
|
/gcbrun |
a24a274
to
838ca62
Compare
/gcbrun |
There was a problem hiding this 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. 👍🏻
install/helm/agones/values.yaml
Outdated
@@ -282,6 +286,32 @@ agones: | |||
totalRemoteAllocationTimeout: 30s | |||
allocationBatchWaitTime: 500ms | |||
topologySpreadConstraints: [] | |||
processor: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
/gcbrun |
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. |
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. |
/gcbrun |
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. |
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. |
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. |
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. |
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. |
49b1bac
to
eb00606
Compare
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:
|
/gcbrun |
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:
|
77233cb
to
eb00606
Compare
Rolled back the fix of the pipeline to keep it on a separate task |
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:
|
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non blocking suggestion)
// 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: a9b200f
cmd/processor/main.go
Outdated
signals.NewSigTermHandler(func() { | ||
logger.Info("Pod shutdown has been requested, failing readiness check") | ||
cancelCtx() | ||
time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: a9b200f
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. |
/gcbrun |
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. |
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:
|
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: