Repository navigation
Remove CountsAndLists feature gate from controllers, allocation, auto… - #4760
markmandel merged 7 commits into
Conversation
…scaler and metrics
github-actions
Bot
added
kind/cleanup
|
Build Succeeded 🥳 Build Id: b32fb3de-0519-4e24-9ddf-ead7d1e26858 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: |
|
Looks like a solid breaking out of work in a single PR - nice job. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The runtime changes consistently remove the targeted gate, with only a minor spelling correction outstanding.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Removes CountsAndLists feature-gate checks from controllers, allocation, autoscaling, sorting, conversion, and metrics paths.
Changes:
- Makes counter/list aggregation, allocation, sorting, autoscaling, and metrics unconditional.
- Updates unit tests by removing feature-gate setup and obsolete disabled-state cases.
| File | Description |
|---|---|
pkg/metrics/controller.go |
Always records counter/list metrics. |
pkg/metrics/controller_test.go |
Removes metric feature setup. |
pkg/gameserversets/gameserversets.go |
Always applies counter/list priorities. |
pkg/gameserversets/gameserversets_test.go |
Removes sorting feature setup. |
pkg/gameserversets/controller.go |
Always aggregates counter/list status. |
pkg/gameserversets/controller_test.go |
Updates aggregation expectations. |
pkg/gameserverallocations/find.go |
Always honors allocation priorities. |
pkg/gameserverallocations/find_test.go |
Removes disabled-feature setup. |
pkg/gameserverallocations/allocator.go |
Always processes counter/list allocation data. |
pkg/gameserverallocations/allocator_test.go |
Removes feature-specific setup. |
pkg/gameserverallocations/allocation_cache.go |
Always uses priorities for tie-breaking. |
pkg/gameserverallocations/allocation_cache_test.go |
Removes sorting feature setup. |
pkg/fleets/controller.go |
Always propagates and aggregates counter/list data. |
pkg/fleets/controller_test.go |
Removes obsolete gated-status tests. |
pkg/fleetautoscalers/fleetautoscalers.go |
Always permits counter/list policies. |
pkg/fleetautoscalers/fleetautoscalers_test.go |
Removes gate-dependent policy cases. |
pkg/allocation/converters/converter.go |
Always converts counter/list fields. |
pkg/allocation/converters/converter_test.go |
Removes converter feature setup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if sortKey == uint64(0) { | ||
| sortKey = newSortKey | ||
| } | ||
| // SortKey returns the sorting values (list of Priorities) as a determinstic key. |
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
Please also fill in the PR description, and note that the pkg/sdkserver gate checks are left for the follow-up PR.
| t.Parallel() | ||
|
|
||
| // TODO: remove when `CountsAndLists` feature flag is moved to stable. | ||
| runtime.FeatureTestMutex.Lock() |
There was a problem hiding this comment.
This TODO is about removing the runtime.FeatureTestMutex.Lock() for tests that no longer call ParseFeatures. Either add back the todo, or remove the unneeded locks.
| "CounterActions increment and ListActions add, delete, and update capacity": { | ||
| features: fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists), | ||
| gs: &gs1, | ||
| gs: &gs1, |
There was a problem hiding this comment.
With the last features value gone, the features related code in this test can be removed too.
features string
...
runtime.FeatureTestMutex.Lock()
defer runtime.FeatureTestMutex.Unlock()
// we always set the feature flag in all these tests, so always process it.
require.NoError(t, runtime.ParseFeatures(testScenario.features))
|
Build Succeeded 🥳 Build Id: fbd78c86-6a90-4d1d-bf34-e1ec316b7af8 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: |
|
I've seen 2 places where it's still using the Otherwise LGTM ! Nice one |
Yes, this scenario is part of another PR. |
|
Build Succeeded 🥳 Build Id: 43ed7e1f-94d3-46c2-8e12-49897419453f 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: |
|
Build Succeeded 🥳 Build Id: 8fa187ff-8e73-488f-a440-2d1c313665e7 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: |
|
This looks good - sorry for the delay, been sick. Can we fix up the conflicts, and then I'm happy to merge 👍🏻 |
|
Build Failed 😭 Build Id: 0f3eccd0-1c72-4af2-8aff-3d04f01cd4c2 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 🥳 Build Id: e0e75509-9c7b-4bcf-9539-9c6fa3450776 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 cleanup
What this PR does / Why we need it:
Part of graduating
CountsAndListsto Stable. This follows the API validation PR (#4733) and removes theFeatureCountsAndListsgate checks from the controller and allocation code paths, so Counters, Lists and Priorities are always processed:pkg/allocation/converters: always convertPriorities,CountersandListsbetween the allocation API and the CRD types.pkg/gameserverallocations:find.SortKeyin the allocator.ListSortedGameServersstays; only the gate check is removed.pkg/fleetautoscalers: remove the "feature flag not enabled" error forCounterPolicy/ListPolicy.pkg/fleets: always syncPrioritiesto the active GameServerSet and aggregate Counter/List status.pkg/gameserversets: always aggregate Counter/List status, and always use Priorities when sorting for Packed and Distributed.pkg/metrics: always record Fleet Counter and List metrics.Tests:
ParseFeaturescalls and theFeatureTestMutexlocks that guarded them where a test no longer changes feature flags.Which issue(s) this PR fixes:
Works On #4590
Did you use AI tools in preparing this PR?:
Y
Special notes for your reviewer:
FeatureCountsAndListschecks inpkg/sdkserver(sdkserver.go,sdk.go,localsdk.go) are left for the follow-up PR. The gate definition inpkg/util/runtime/features.gostays until those checks are removed too.CountsAndLists=false. They no longer clear Counter/List status on Fleets and GameServerSets, and the autoscaler no longer rejects Counter/List policies when the gate is off.go test -raceoverpkg/...,sdks/go/...andcmd/....