Skip to content
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

Counters: Update Default Capacity #3594

Closed
wants to merge 3 commits into from

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Jan 17, 2024

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking

/kind bug

/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3579

Special notes for your reviewer:

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 1672ec84-b184-498d-ac94-fd4f38d9f773

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e0f268de-d427-435c-864b-97fe61afb59c

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

@igooch
Copy link
Collaborator

igooch commented Jan 18, 2024

Try running make gen-install from within the agones/build directory.

@Kalaiselvi84
Copy link
Contributor Author

shall I revert the change in install/yaml/install.yaml and try make gen-install?

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review January 19, 2024 00:28
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 90168406-8321-4437-bb58-206c3dfaa297

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/3594/head:pr_3594 && git checkout pr_3594
  • 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.38.0-dev-eab2df1-amd64

@Kalaiselvi84
Copy link
Contributor Author

Kalaiselvi84 commented Jan 19, 2024

I have tested with Counters default capacity with 9223372036854775807 and 0.

Is 0 correct for the Counters default capacity?

The Counters info are missing in the status with 9223372036854775807

Counters are present with 0

@igooch
Copy link
Collaborator

igooch commented Jan 22, 2024

The TL;DR is currently our max capacity per game server = max(int64) / replicas.

The fleet aggregates the count and capacity of all the game servers counters in the fleet. So if game server counter capacity = max(int64) and there are 3 replicas in the fleet, then the fleet aggregate counter capacity = 3 * max(int64) which errors. When I put in max(int64) / 3 as the max capacity for a game server, then the fleet and game servers started up just fine.

We'll need to change the code, and documentation, so that if the aggregate capacity or count for the fleet goes above max(int64), then the aggregate would be max(int64), so that we don't get an error on aggregation. We should probably do this as a separate PR before updating the defaults.

@igooch
Copy link
Collaborator

igooch commented Jan 22, 2024

Opened #3604 to address the aggregating past int64 issue.

Copy link
Member

@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.

Just holding off on this one until we decide if we should make count capacity nullable and the Go model *capacity to indicate essentially unlimited capacity.

@igooch @zmerlynn WDYT? (from chat discussion)

@Kalaiselvi84
Copy link
Contributor Author

This link shows the example using big.NewInt to handle the larger number.

@Kalaiselvi84
Copy link
Contributor Author

@markmandel, @zmerlynn, and @igooch, have we decided on updating the default capacity for the Counters or keeping it as is?

@markmandel
Copy link
Member

markmandel commented Feb 6, 2024

Just checking - but @igooch @zmerlynn we can merge this as is now?

If I really want the max capacity to be represented by nil once I've looked at the developer experience, I'll roll up my sleeves and make the chage.

Sound good?

@igooch
Copy link
Collaborator

igooch commented Feb 6, 2024

We can't have this default to max(int64) because of the issue with jsonpatch being lossy. So we'll need some other arbitrary value for default.

@Kalaiselvi84
Copy link
Contributor Author

Closing this PR and focusing on the documentation updates in PR #3637

@Kalaiselvi84 Kalaiselvi84 deleted the pr-3579 branch March 15, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Counters: Default capacity should be 0 (max), not 1000
4 participants