-
Notifications
You must be signed in to change notification settings - Fork 823
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
Prevent Int64 Overflow #3605
Prevent Int64 Overflow #3605
Conversation
Build Failed 😱 Build Id: b886e664-8de1-4115-bf7c-de28307cdbc5 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: ecdf5262-fa85-4f70-86a0-f56d37ff858c 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.
Could you also add in unit tests for the functionality? They can go in TestControllerUpdateFleetCounterStatus
for the fleet controller and TestComputeStatus
for the gameserverset controller.
pkg/fleets/controller.go
Outdated
@@ -721,6 +722,14 @@ func (c *Controller) filterGameServerSetByActive(fleet *agonesv1.Fleet, list []* | |||
return active, rest | |||
} | |||
|
|||
// SafeAdd prevents overflow by limiting the sum to math.MaxInt64. | |||
func SafeAdd(x, y int64) int64 { |
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 function should probably go in pkg/apis/agones/v1/common.go
Build Failed 😱 Build Id: 2877393a-b061-4dc9-8f0d-ab82a8ec57d9 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: a354c8fb-a61e-42d0-841b-928831cb43f8 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: 9528b86c-8336-4724-8768-a1a9d4a3fef9 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.
For testing some edge cases you can change
gsSet1.Status.Counters = map[string]agonesv1.AggregatedCounterStatus{
"fullCounter": {
AllocatedCount: 1000,
AllocatedCapacity: 1000,
Capacity: 1000,
Count: 1000,
},
To have maxInt64:
gsSet1.Status.Counters = map[string]agonesv1.AggregatedCounterStatus{
"fullCounter": {
AllocatedCount: 9223372036854775807,
AllocatedCapacity: 9223372036854775807,
Capacity: 9223372036854775807,
Count: 9223372036854775807,
},
pkg/fleets/controller_test.go
Outdated
assert.Equal(t, int64(30), fleet.Status.Counters["thirdCounter"].AllocatedCapacity) | ||
assert.Equal(t, int64(400), fleet.Status.Counters["thirdCounter"].Capacity) | ||
assert.Equal(t, int64(21), fleet.Status.Counters["thirdCounter"].Count) | ||
assert.Equal(t, agonesv1.SafeAdd(1000, 100), fleet.Status.Counters["fullCounter"].AllocatedCount) |
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.
Here we want to leave assert.Equal(t, int64(21), fleet.Status.Counters["thirdCounter"].Count)
. The err := c.updateFleetStatus(ctx, fleet)
will call the SafeAdd as part of the method, so that's part of what we're testing.
AllocatedCapacity: 10, | ||
Count: 50, | ||
Capacity: 85, | ||
AllocatedCount: agonesv1.SafeAdd(5, 0), |
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.
For these we'll want to leave as plain int64.
You can add another Counter under multiple game servers similar to:
gs1.Status.Counters = map[string]agonesv1.CounterStatus{
"firstCounter": {Count: 5, Capacity: 10},
"secondCounter": {Count: 100, Capacity: 1000},
"fullCounter": {Count: 9223372036854775807, Capacity: 9223372036854775807},
}
And under expected add:
"fullCounter": {
AllocatedCount: 9223372036854775807,
AllocatedCapacity: 9223372036854775807,
Count: 9223372036854775807,
Capacity: 9223372036854775807,
},
A general comment is that we noted there's an issue with jsonpatch here https://github.com/googleforgames/agones/blob/eb4035e1f78e7a8e0c147a013a5405f904ab9899/pkg/gameservers/controller.go#L260C21-L273 that's preventing the game server from taking an actual max(int64) value. There's a fix in a different json package evanphx/json-patch#194, but we need to incorporate that into our code. All that is to say this code mostly looks good, but the game server won't currently take max(int64) due to a separate bug. I don't think this PR needs to wait on that code, we can solve that issue separately. |
+1 on this. |
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.
LGTM me too. Can come out of draft.
Build Succeeded 👏 Build Id: 278dca47-49ac-466d-b094-008344c3b93a 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 bug
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #3604
Special notes for your reviewer: